Repository navigation
feat(libSceLibreSsl3): implement ASN.1 integer decoding - #10948
Conversation
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
Ready for review at Please run maintainer-triggered full Linux and Windows CI for this head. Once review and CI are satisfactory, please merge. #10960 declares this PR first for shared-file integration, and I will rebase it after this merges. |
ahmadmuj
left a comment
There was a problem hiding this comment.
Reviewed head 8b87391b3143fa5568191659bbd03c7ac5ca2554, merged locally onto main 29ce6d87eb5b3050b4f08f60ed9ee52195979d14 (clean; still clean on 4017310678562c63594d99b8f45db2d68b9aaa6c, which touches nothing in libSceLibreSsl/libSceLibreSsl3).
What I ran, Linux x86-64, GCC 13.3:
- Merged tree, full configure with
BUILD_TESTING=ON, built only the three libSceLibreSsl3 test targets:guest_crypto_memcmp,guest_libressl3_digest,guest_libressl3_asn1pass 3/3.check_conventions.py --base origin/mainpasses. - A differential against real LibreSSL 4.0.0: portable tarball (SHA256 checked against the OpenBSD list), static
libcryptobuilt from source,d2i_ASN1_INTEGERvs yourIntegerDecoder.cppunmodified (onlymalloc/free/NotImplementedshimmed), ASan+UBSan on. It compares success vs NULL/throw, type, length, flags, data bytes and the*inadvance, and on failure that*indidn't move.- Exhaustive: every 1-3 byte content under
02 0n, plus every identifier byte x length byte x a few first content bytes at every truncatedinlen. - Sign padding at sizes 2-40 (
00 80..,ff 00..,ff 80..,00 00 01,ff ff 01, ...), each with long-form lengths81-84,88,89andfewith leading zero length bytes, trailing bytes and every prefix. - Headers:
1f 02,1f 80 02,1f 82 00, tag overflow, constructed and non-universal classes,80,ff, huge lengths, length > buffer,inlen0 and negative. - 2.5M random cases.
- 21.8M cases total, 0 mismatches, 0 sanitizer reports. A scratch mutant without the redundant-padding check gives 541k mismatches (first one
02 02 00 05), so the harness does see those paths.
- Exhaustive: every 1-3 byte content under
- Replacement with a non-null
*a(an earlier decode of02 02 12 34): on success both free the old struct and its data and set*ato the new value. Negativeinlen: both return NULL and leave*aalone, so thatreturn nullptrmatchesASN1_item_ex_d2i(tasn_dec.c L1250) and isn't a silent stub. - Walked it against the pinned LibreSSL sources too:
asn1_get_identifier_cbs/asn1_get_length_cbs(asn1_lib.c L24-137),c2i_ASN1_INTEGER_cbsand the twos/padding helpers (a_int.c L528-737), andASN1_STRING_free(a_string.c, NDEF = 0x10, freezero). Everything lines up. The one ordering difference: LibreSSL frees*abefore the content checks, so02 02 00 01with an existing*areturns NULL with*afreed and set to NULL. Your debt line already covers that, and you throw there anyway.
Approving. Two small notes, not blocking:
- Since 8b87391 there's no test for the content rejections (empty content,
02 02 00 7f-style redundant padding), so dropping that check would still pass the suite. I get why you don't want to freeze the throw paths. Just flagging that the differential above is the only thing covering them right now. docs/dev/debt/libSceLibreSsl3.md: "ASN.1 integer decoding ... have no PS5 verification" should be "has".
Limits: no dump or PS5, so the 24-byte layout and app-heap allocation are only checked against LibreSSL's ASN1_STRING and the existing Asn1String in libSceLibreSsl.
8b87391 to
77ba690
Compare
|
Changed “have” to “has” in the ASN.1 decoding debt entry. The unfinished-error assertions remain removed. Rebased onto main 4017310; the entire libSceLibreSsl3 tree and both external library tests are byte-for-byte identical to reviewed 8b87391. Conventions and diff checks pass. No runtime rerun was needed for this documentation-only correction. Updated head: |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 77ba69041c5ec098041c598c9a23cbcac15f0d73. The only change since the reviewed implementation is the grammar correction in the ASN.1 debt entry; the implementation and tests are unchanged. I ran python3 tools/check_conventions.py --base upstream/main --head HEAD and git diff --check upstream/main...HEAD; both pass. The author reports no runtime rerun for this documentation-only change. The earlier exact-head Linux focused suite and independent LibreSSL differential review remain applicable. #10960 declares this PR first for the shared-file ordering.
|
@AZLisme firmware 13.42 data for
|
|
@BunnyFl I’ve credited the native-PC comparison in Tested and clarified the debt in The rejected-input difference remains: this decoder throws, while the native module returns NULL with ASN.1 errors. The legacy queue exists, but the decoder does not populate it. Exact general error sequences, prior-output frees and allocation/NULL/failure ownership remain unverified, so the category table alone does not justify a behavior change. This commit changes documentation only; the complete |
|
@AZLisme thanks for adding it. Merging isn't something I can do: I'm not a maintainer. Here is firmware 13.42 data for the three points the debt entry still lists, from the decrypted One correction to my earlier table: its code lists were the distinct first queued codes per category, not sequences. Full sequences are below. Error sequences: the same input categories as before, every queued code read back. That was 60,000 inputs in three identical runs, plus 200,000 with a second seed, which gave the same set. A successful decode queues nothing. Every NULL return queues one of these exact sequences:
An earlier object in
Allocation failures: a valid positive and a valid negative encoding, with and without an old object, failing each allocation in turn. The module makes two allocations, the data first and then the object.
NULL pointers and negative lengths:
|
|
@BunnyFl Thank you for the full sequences, allocator recordings and controls, and for clarifying that you are not a maintainer. In The attribution remains firmware 13.42 running natively on a PC with a recording allocator, not physical-console/application-heap validation. Nonnegative NULL input remains a guarded throw rather than reproducing the module fault. This commit changes documentation only: every non-debt file, including the full |
|
Maintainer run 40382921477, attempt 1, passed for unchanged The description records this exact composite result while retaining the native-PC attribution and remaining NULL/error-queue/old-output lifetime gaps. No source changes or console-parity claim. Please re-review this head and merge when the usual requirements are satisfied. |
What
Implement
d2i_ASN1_INTEGERfor arbitrary-width signed integers, BER forms, cursor advancement, replacement and guest-heap ownership. The canonical public declaration uses the existing 24-byte ASN.1 string layout. Replacing a prior value uses private cleanup, so no additional public accessor/free export is required.Contract: LibreSSL 4.0.0 integer decoding and string lifetime. Decoder error-queue paths throw and remain documented in debt; the existing legacy queue is not populated by this decoder. Public
ASN1_INTEGER_freeandASN1_STRING_get0_dataremain unchanged throwing exports; their implementation is deferred while #10452 is reviewed. This decoder does not depend on #10452.Tested
Maintainer run 40382921477, attempt 1, completed successfully for unchanged head
81013f41bf09c3d543bb110d4954abb767df0229. Linux and Windows used the exact main baseafc780e681fc379a6ed1b19a2a247cae17f0fc07with this complete ordered five-PR composition:81013f41bf09c3d543bb110d4954abb767df0229c66f61ff5259e3d05eab0e35c99b948d2b62903bf8a99b6b8a29f00894e9e552900c0d67d2afa259e36c33c82503fe956b889187e1c10bbe71b0d663c9ffec68f8cb9a9e10bf89797acc662624677224Windows: 1,689 passed, 815 expected skips, zero failures out of 2,504 registered CTests. Linux: 2,305 passed, 93 expected skips, zero failures out of 2,398. Both skip validators executed and accepted every skip. The normal builds and production
libstargets succeeded; both logs showIntegerDecoder.cppcompiled and the actual LibreSSL3 module linked and NID-patched.All three ASN.1 fixtures passed on each platform:
guest_libressl_asn1,guest_libressl_asn1_allocatorandguest_libressl3_asn1. Each platform also passed all 65 public-header cases (declaration audit plus 64 compile batches). Registration audits found 1,404 executable targets on Windows and 1,409 on Linux, zero unregistered; these are registration counts, not executed-test counts. All 12 workflow jobs completed successfully. Supplemental matrix results do not establish macOS ASN.1 product coverage or library-ASan validation.This qualifies the recorded five-PR union, not the standalone PR, a later main, physical-console parity, or fixes for the decoder’s remaining NULL/error-queue/old-output lifetime differences. This update changes the description only. The existing source, dependencies and debt remain unchanged. Statements below about no new runtime results or absent Windows/full builds describe their original historical follow-ups and local qualification.
Contributor evidence: BunnyFl compared the unchanged
77ba69041cdecoder with decrypted firmware 13.42 running natively on a PC, expressly not a physical console. For the 147,982 nonthrowing cases in a 200,000-input run, returned values, decoded fields, cursor advancement and output replacement matched; the report describes three identical runs and a further 500,000-input seed with no comparison mismatches. For 52,018 throwing cases, the native module returned NULL and queued errors. The earlier category table listed distinct first error codes, not complete sequences.The follow-up report supplies full sequences and recording-allocator observations: 60,000 error inputs in three identical runs and 200,000 with a second seed produced the same sequence set. Tested successful replacement frees owned data after clearing it, then the old object, before allocating new data/object; flag
0x10preserves borrowed data. The module and decoder match that order and tested allocation-failure cleanup. Native first-allocation failure returns NULL without errors; second-allocation failure returns NULL with0x0DFFF041twice, while this decoder throws. Redundant-sign/zero-content rejection queues0x0DFFF03Eand frees/clears old output natively, while this decoder throws and retains it. Other tested parse failures preserve old output but return NULL with ordered errors natively. Negative lengths return NULL with no queued error and preserve input/output on both. Nonnegative NULLin/*infaults in the module and throws here. The successful-replacement comparison covered 20,000 inputs in three identical runs plus 79,934 second-seed decodes; allocation and NULL/negative tests also repeated three times, with mutation controls for zeroing/free order.These are contributor-reported native-PC results with an allocator stand-in, separate from our tests below. They confirm the tested ownership/unwind order and specific remaining differences; they do not establish physical-console/application-heap interoperability or exhaust all malformed/overflow error precedence. The decoder still does not populate the existing legacy queue, and the remaining LibreSSL3 ERR exports still throw. This follow-up updates the debt entry only; decoder/library/test sources and historical runtime qualification remain unchanged. No new runtime result is claimed for this documentation update.
Documentation follow-up
81013f41bf09updates only the ASN.1 debt entry for the follow-up native-PC report, stating the remaining error-return and old-output lifetime differences first. Every non-debt file, including the entirecoretree, is byte-identical to816ca46c66e9; no decoder, API, allocator, error-queue or test behavior changes. Conventions and whitespace checks pass. A private static merge with main4e43321bd6ceccdc1ae28568bbbc35d9a6446464is clean; this is a source-union check, not merged-main runtime qualification. Existing runtime evidence below retains its original scope, without a documentation-only rerun.Documentation follow-up
816ca46c66e9qualifies the new native-PC evidence and decoder/error-queue gap in debt. The entirecoretree is identical to77ba69041c5e; only the debt row changes. Existing runtime evidence is retained without a documentation-only rerun. Conventions, whitespace and independent documentation/source review pass, and this revision merges cleanly with main504ea3b53b23in a static check.Review follow-up
77ba69041c5erebases onto401731067856and fixes the debt-entry grammar only. The full LibreSSL3 library/test tree and external test sources are byte-identical to approved8b87391b3143; conventions and whitespace checks pass. The focused runtime results below are retained from that unchanged source, without a documentation-only rerun.Linux x86_64, GCC 16.2.1, Release, based on
63a863d3b07f2457ee1210f1abba1c11409cdff5. The complete changed runtime library was built through its normal module CMake, using freshly rebuilt libc/libkernel/libSceLibcInternal from that base. Public-header uniqueness/umbrella audit and duplicate-inclusion syntax compilation passed; conventions and whitespace checks passed. Windows/macOS, PS5 execution and a monolithic repository build were not run. All 3/3 focused tests passed: crypto comparison, existing LibreSSL3 digest, and INTEGER decoding. Decoder coverage includes all 65,536 signed 16-bit values, larger values, successful BER forms, input advancement and preservation, replacement of owned/borrowed values, and secure clearing before release. A fresh original-source check also passed three null-data replacement cases with old lengths 0/-1/INT_MIN. The original pinned library previously matched 65,562 valid decoding cases.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)