Repository navigation
fix(libSceLibreSsl3): return d2i_ASN1_INTEGER errors instead of throwing - #14171
oneandonlydean merged 2 commits into
Conversation
Rejected encodings and allocation failures now return NULL with the error queue, the old output and the input pointer that firmware 13.42's module and LibreSSL 4.0.0 leave. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CodingBreadd
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The decoder matches LibreSSL 4.0.0 and the 13.42 measurements on every tested path. The one untested path, content over INT_MAX bytes, does not do what the PR body and debt entry say: it frees the caller's old object, but LibreSSL leaves it alone.
What I ran
Reviewed head 302eb1fb4c81b46a57396645cf1f3c834dafe6c3 against base f776fddcffd1cfece220ecdd894371988b164f6a. That base is also current main, and the merge preview was clean. Environment: Windows 11 x86-64, Python 3.14.7. I did not build or run CTest locally.
git diff --check f776fddcffd1cfece220ecdd894371988b164f6a...302eb1fb4c81b46a57396645cf1f3c834dafe6c3
python tools/check_conventions.py --base f776fddcffd1cfece220ecdd894371988b164f6a --head 302eb1fb4c81b46a57396645cf1f3c834dafe6c3 --title 'fix(libSceLibreSsl3): return d2i_ASN1_INTEGER errors instead of throwing' --body pr-body.md
git merge-tree --write-tree origin/main 302eb1fb4c81b46a57396645cf1f3c834dafe6c3
gh run view 41246077625 --job 124671162151 --log | grep -E 'libressl3_asn1|tests passed'I also read the pinned LibreSSL ce488b78e0bd sources (asn1/tasn_dec.c, asn1/asn1_lib.c, asn1/a_int.c, asn1/a_string.c) side by side with IntegerDecoder.cpp.
Results
| Check | Result |
|---|---|
| Diff whitespace check | Passed; no output |
| Repository conventions and PR template | Passed |
| Current-main merge preview | Clean; no declared dependencies |
| Source comparison: header | ReadIdentifier/ReadLength/ReadInteger match asn1_get_identifier_cbs, asn1_get_length_cbs and asn1_check_tag: same order (identifier → tag/class → length → primitive-indefinite → TOO_LONG), same overflow guards, same 0x7f reserved rejection. NESTED_ASN1_ERROR is pushed only for header failures, as asn1_d2i_primitive does. |
| Source comparison: content | The TYPE_NOT_PRIMITIVE-after-length order, the free-before-empty-check order, the asn1_aint_keep_twos_padding rule (FF 00… kept) and the redundant-sign rejection all match asn1_d2i_primitive_content and c2i_ASN1_INTEGER_cbs. Two MALLOC_FAILUREs match ASN1_STRING_type_new plus asn1_item_ex_new. FreeInteger matches ASN1_STRING_free/ASN1_STRING_clear, including the ASN1_STRING_FLAG_NDEF (0x10) handling. |
| Test coverage | New Rejections and AllocationFailures cover every row of the PR table, with and without an old object. |
| Exact-head hosted test | Windows job on 302eb1fb4c81 built and passed guest_libressl3_asn1, with 2539/2539 passing. Linux and the specialized jobs were skipped; labels was still pending at the snapshot. |
| Author-reported negative controls | Four reverted mutations failed the test; I did not reproduce them. |
Unedited local output
All conventions checks passed.
git diff --check produced no output and exited 0.
c4959dec4b43348472f997e4e9c59c65673ad9ee
1834/2539 Test #2284: guest_libressl3_asn1 ..................................................... Passed 0.03 sec
100% tests passed, 0 tests failed out of 2539
Findings
Blocking
-
Content over
INT_MAXbytes frees the caller's old object; LibreSSL leaves it untouched. In LibreSSL 4.0.0,asn1_d2i_primitive_contentcallsasn1_c2i. That function returns 0 whenCBS_len(content) > INT_MAX, beforec2i_ASN1_INTEGER_cbsruns. So for that input the old*outis not freed,*outis not set to NULL, nothing is allocated and nothing is queued. Thedata_len > INT_MAXcheck insidec2i_ASN1_INTEGER_cbs(a_int.c#L187) is a second check thatd2inever reaches. At this head the check sits onIntegerDecoder.cpp:167, afterFreeInteger(*output); *output = nullptr;(line 143) and after a full-sizemallocandmemcpy(line 164). Given a 2³¹-byte integer and an old object, the export frees the caller's object and nulls*out, while LibreSSL returns NULL and leaves both alone.The PR body says this path "follows LibreSSL (old output freed, NULL, nothing queued)", and the debt entry says it follows LibreSSL 4.0.0, so the code and both documents disagree with the pinned source. Real inputs almost never reach this path, but the documented contract is wrong, and the fix also removes a needless 2 GiB copy. Reject before touching the output, and drop the later check:
auto content = ReadInteger(remaining); if (!content) return nullptr; if (content->size() > static_cast<std::size_t>(std::numeric_limits<int>::max())) return nullptr; if (output) { FreeInteger(*output); *output = nullptr; }
Then correct the PR body's sentence. After this change the path never reads the content bytes, so a test can reach it: reserve a little over 2 GiB without committing it (
VirtualAlloc(MEM_RESERVE)/mmap(PROT_NONE)), commit only the first page, write a02 84 80 00 00 00header and pass a matching length. If you would rather not add that test, keep the "not tested" wording in the debt entry.
Non-blocking
None found.
…tput asn1_c2i in LibreSSL 4.0.0 rejects content over INT_MAX bytes before c2i_ASN1_INTEGER_cbs runs, so the old output stays and nothing is allocated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks, you're right: |
|
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.
|
Stale: fixed in 63a2430. The over-INT_MAX check now runs before the caller's old object is freed, and a test covers it.
What
d2i_ASN1_INTEGERin libSceLibreSsl3 threw on every rejected encoding and on allocation failure. It now returns NULL with the error queue, old output and input pointer the module leaves.The behaviour follows the firmware 13.42 measurements already linked from the debt entry, taken by running the decrypted module natively on a PC, not on a console. The parsing order follows LibreSSL 4.0.0
asn1_check_tag,asn1_d2i_primitive_contentandc2i_ASN1_INTEGER_cbs.*out0x0DFFF0A8,0x0DFFF03A0x0DFFF066,0x0DFFF03A0x0DFFF09B,0x0DFFF03A0x0DFFF0E70x0DFFF03E0x0DFFF041twice*indoes not move on a NULL return. A nonnegative length with a null input still throws, because the module faults there.The old object is now freed before the empty and redundant-sign checks, not after them, as the measurements show. Errors go on libSceLibreSsl's queue through
LibreSslPutError_nid_no_patch.One path has no measurement: content over
INT_MAXbytes follows LibreSSL, whereasn1_c2irefuses it beforec2i_ASN1_INTEGER_cbsruns (NULL, nothing queued, nothing allocated,*outuntouched). The debt entry records this and is cut to what is still open.guest_libressl3_asn1_testsnow also linkslibSceLibreSsl, to read the queue.Tested
Windows 11, WinLibs GCC 15.2.0, Release, on this branch. No title or homebrew was run, and I found no title that hits these paths.
ctest -R 'libressl|guest_crypto_memcmp|^public_headers': 102 of 102 passed on the first commit. After theINT_MAXfix,ctest -R 'guest_libressl3_|guest_crypto_memcmp|^public_headers': 73 of 73 pass, includingguest_libressl3_asn1.0x0DFFF041once, checking constructed before the length, and checkingINT_MAXafter freeing the old object.guest_libressl3_asn1failed for all five.check_conventions.py --base upstream/mainpasses.Not run: the full test suite, Linux, macOS, or any comparison with a console.
pr_overlap.pyexited non-zero because six PRs could not be compared automatically: #9970, #10461, #10557, #10631, #10960 and #12928. They shareCMakeLists.txtor the debt file. I compared them by hand and none edits the ASN.1 test link line or the ASN.1 integer debt entry. No open PR changesIntegerDecoder.cpportests/Asn1.cpp.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)🤖 Generated with Claude Code