x509: read the ASN.1 tag after the length check, not before

[tinic]

_nx_secure_x509_asn1_tlv_block_parse() loaded buffer[0] into current_tag one statement before testing *buffer_length < 1, so every caller that runs out of data read one byte past the end of the buffer it was given.

It is reachable from the wire. A certificate two bytes long reaches it through _nx_secure_x509_certificate_parse(), and so does the issuer walk in the certificate store, which calls the parser repeatedly as it consumes a chain and hands it whatever remains.

Found by a fuzz driver over _nx_secure_tls_process_remote_certificate() with a real DER corpus, and confirmed under AddressSanitizer.

The read is moved below the guard. The guard already returned the right status; only the load was in the wrong place.

[fdesbiens]

Thank you — this is a clean find and the smallest possible correct fix. I verified it rather than reading it, because "reachable from the wire" is a claim worth testing.

The over-read is real, and I reproduced it separately from your fuzzer. I extracted the function verbatim, modelled the caller that has consumed its whole buffer by passing the tail pointer with a remaining length of zero, and put the allocation at exactly the length the parser is told about so ASan would trap:

dev   (tag read before guard): AddressSanitizer: heap-buffer-overflow
                               READ of size 1
                               #0 tlv_dev  ... current_tag = buffer[current_index];
fixed (tag read after guard) : returned 0x182, no ASan report

0x182 is NX_SECURE_X509_ASN1_LENGTH_TOO_LONG, so the guard was already returning the right thing and, as you say, only the load was misplaced.

Reachability holds structurally, not just via the fuzzer. nx_secure_x509.c:1044 and :1065 are consecutive calls with no zero-length check between them: the first decrements *buffer_length by header plus value and the caller advances current_buffer by the same amount, so if that consumed the remainder the second call gets a tail pointer and a length of zero — exactly the shape I reproduced. Worth contrasting with the OID loop at :852, which is wrapped in while (length > 0) and is therefore safe; the unguarded sequential calls are the ones that matter.

The fix is complete for this function. I checked the rest of it for the same pattern and found none. The multi-byte length path is correctly ordered: length_bytes > 4 || length_bytes > *buffer_length at :201 rejects before the read loop, and the final length > *buffer_length check at :229 guards the value. The second *buffer_length < 1 guard before length = buffer[current_index] is also on the right side of its read. So the one you moved was the only one out of place.

On impact, so the record is accurate rather than alarming: the byte read is one past the end and is then used as an ASN.1 tag, so it steers which error the parser returns. It is not returned to the peer, so there is no disclosure path, and a one-byte over-read immediately past a buffer will not fault on the flat memory maps typical of the targets this stack runs on.

[fdesbiens]

You describe a fuzz driver over _nx_secure_tls_process_remote_certificate() with a real DER corpus, run under AddressSanitizer. There is nothing like that in the tree today: test/regression/nx_secure_test/ is entirely hand-written cases with fixed inputs, and certificate parsing is precisely the kind of deep recursive-descent code over hostile input where fuzzing finds things that hand-written tests structurally cannot. This bug is the demonstration — it had been reachable for as long as the parser has existed and no test found it.

If you are able to share it, I would like to look at what it would take to carry it here, even if only as a manually-run harness rather than something wired into CI. The corpus is the awkward part, since a useful DER corpus is not small, but even the driver alone plus instructions would let others reproduce and extend your work.

If it is entangled with your downstream build or you would rather not, that is entirely fine, and it does not affect this PR. I would still be interested to know roughly what shape it takes — libFuzzer, AFL, something bespoke — because that tells us what a project-side equivalent would need to look like.

[fdesbiens]

For consistency with the other reviews in this batch, where I have asked for tests on most changes.

A test here would have to construct the state the bug needs: a parser call with a tail pointer and a zero remaining length. Reaching that through the public API means crafting a certificate whose inner lengths exhaust the buffer at exactly the right nesting depth, which is a fragile thing to encode as a fixed input and would break the moment the parse order changed. Calling the parser directly with a zero length is easy, but the read is only detectable with a sanitiser and an exact-size allocation — which is what my harness did and what the project's test builds are not configured for, so the test would pass either way and prove nothing.

The durable answer to this class is not a case per bug; it is the fuzzer. That is where I would rather spend the effort, and it is why I am asking for the driver instead of a test.

[fdesbiens]

Thank you. I will merge this into dev in the next few days, along with other PRs from this batch.

This will be part of the Q3 2026 release in September.