asn1: decode BER integers with int.from_bytes

[KernelClint]

BERcodec_INTEGER.do_dec builds the value by shifting a Python integer left one octet at a time at
scapy/asn1/ber.py:464-471.
Each shift copies the whole accumulated integer, so the cost of an octet rises with the number of
octets already read and decoding is quadratic in the encoded width.

Nothing in BER limits that width, so any protocol using it can be made to spend the time. A sender
pads an INTEGER with leading sign octets, which do not change the value, and the parser does the
work anyway. Kerberos EncryptedData.etype is one way in; it is not the only one.

Measured on one machine, decoding one INTEGER:

content octets	before	after
4,000	1.42 ms	0.002 ms
16,000	18.5 ms	0.009 ms
64,000	304 ms	0.032 ms
256,000	5,013 ms	0.137 ms

Quadratic before, linear after.

int.from_bytes(s, "big", signed=True) performs the same two's-complement conversion in one pass.
The two agree on every input tested, including the empty string, single octets on both sides of the
sign boundary, and 20,000 random byte strings.

This replaces the first version of this pull request, which added an Int32 width check to
Kerberos. @gpotter2 was right that the problem is generic rather than Kerberos-specific, so
scapy/layers/kerberos.py is untouched now. Nothing is rejected that was accepted before — wide
integers still parse, just cheaply — which also means certificates and other legitimately large
INTEGER values are unaffected.

The regression test counts that the conversion happens once rather than once per octet, following
the BitLenField test added in #5108. It fails on the unmodified revision.

[codecov]

Codecov Report

❌ Patch coverage is 25.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.48%. Comparing base (ba8641a) to head (6476809).
⚠️ Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
scapy/layers/kerberos.py	25.00%	6 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (ba8641a) and HEAD (6476809). Click for more details.

HEAD has 10 uploads less than BASE

Flag	BASE (ba8641a)	HEAD (6476809)
	12	2

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #5107       +/-   ##
===========================================
- Coverage   80.59%   47.48%   -33.12%     
===========================================
  Files         390      372       -18     
  Lines       96892    96539      -353     
===========================================
- Hits        78094    45844    -32250     
- Misses      18798    50695    +31897     

Files with missing lines	Coverage Δ
scapy/layers/kerberos.py	31.80% <25.00%> (-38.71%)	⬇️

... and 340 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gpotter2]

Correct me if I'm wrong, but while a negative value properly doesn't make sense, it's still whats in the spec: https://www.rfc-editor.org/info/rfc4120/#section-5.2.9. I'm not sure I understand the issue otherwise

[KernelClint]

You're right that negative values are legal, and the patch keeps them — it never looks at the value.
It only bounds how many octets the encoding may use, and it is deliberately looser than the type.

To be precise about what it is: this is a resource bound, not validation of Int32. RFC 4120 §5.2.9
constrains the value to -2147483648..2147483647, which in DER is at most 4 content octets. The
check rejects at more than 5, so it still accepts things that are not legal Int32 — a 5-octet
00 ff ff ff ff decodes to 4294967295 and passes. That looseness is on purpose: the aim was to kill
the pathological arbitrary-width case without tightening Scapy's generally permissive BER decoding
any more than necessary.

The reason for it is cost. BER_num_dec shifts an arbitrary-precision integer once per byte, so work
grows with the square of the declared width, and EncryptedData is reachable from Kerberos TCP
reassembly. Measured on master:

integer bytes	before	after
4,000	1.32 ms	0.01 ms
16,000	17.91 ms	0.01 ms
64,000	281.95 ms	0.02 ms

If a loose bound in a Kerberos-specific field subclass isn't worth it, the reasonable alternatives are
a generic width cap in the BER integer decoder, or nothing at all. Happy either way — the only thing
at stake is the cost, not correctness.

[gpotter2]

BER_num_dec already caps the integer with a maximum value, maybe it needs to be tweaked to a lower one. I think that what is described here is a generic issue (related to ASN.1 BER), rather than something specific to kerberos.

[KernelClint]

You were right, and I have moved it.

The cost is in BERcodec_INTEGER.do_dec, not in Kerberos. It shifts a growing Python integer one octet at a time, so each octet costs more than the last and decoding is quadratic in the encoded width. Kerberos etype was just the field I happened to reach it through — any protocol that decodes a BER INTEGER can be made to spend the same time, by padding it with leading sign octets that do not change the value.

So this version leaves scapy/layers/kerberos.py alone and changes the codec instead. int.from_bytes(s, "big", signed=True) does the same two's-complement conversion in one pass. Decoding 64,000 content octets goes from 304 ms to 0.03 ms, and 256,000 from 5.0 s to 0.14 ms.

On your earlier point about BER_num_dec — that one caps tag and length numbers, which is a different path from the INTEGER value octets this touches, so I left it as it is.

Two things worth saying plainly:

• Nothing is rejected now that was accepted before. The width check is gone. Wide integers still parse, they are just cheap. That also means certificate moduli and other legitimately large INTEGERs are unaffected, which the old approach would have had to carve out.
• The old test could not have passed. It expected a Raw fallback, but test/regression.uts sets conf.debug_dissector = True at the top of the file, so the exception propagated instead. That is why CI was red. It is deleted rather than wrapped, since there is no rejection left to test.

The new test counts that the conversion happens once rather than once per octet, in the same shape as the BitLenField test from #5108. It fails on the unmodified revision.

[gpotter2]

We don't need the massive comment, otherwise OK.

[KernelClint]

Trimmed to one line in 6d8635c.

[KernelClint]

One note on the red Check the validity of the commits job here — it is not this branch.

.config/ci/check_commits.sh passes on my commit and fails on 32852c0f ("Update AGENTS.md with coding and testing guidelines"), which is on master and has no AI-Assisted trailer:

OK:    Commit 151da773... is properly tagged.
ERROR: Commit 32852c0f... is missing the 'AI-Assisted: yes|no [tool(s)]' trailer.

The job fetches three commits deep, so it reaches master and reports it against whichever pull request happens to be running. #5050 is failing the same way. Older runs pass only because they finished before that commit landed.

Nothing for me to change here, but you may want to amend that commit or have the script skip commits already on the base branch.