Fix various reported issues etc.#75
Open
DDvO wants to merge 33 commits into
Open
Conversation
DDvO
changed the title
DDvO
changed the title
DDvO
force-pushed
the
fix_issues_by_ChatGPT
branch
from
cd3a1db to
c574d49
Compare
DDvO
force-pushed
the
fix_issues_by_ChatGPT
branch
from
c574d49 to
80016bf
Compare
DDvO
force-pushed
the
fix_issues_by_ChatGPT
branch
from
80016bf to
d927dea
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens libsecutils against a set of reported correctness and security issues by tightening error handling, adding bounds/overflow checks, and clarifying/documenting security-related behaviors (ICV/DV availability, URL-based fetching, and “trivial TLS” semantics).
Changes:
- Strengthen input/size validation and failure signaling in utility helpers (safe string copy, URL encoding) and crypto (AES-GCM encrypt/decrypt).
- Improve robustness of OCSP/CRL handling (nonce usage gated by build flag, cert ID matching failure handling, safer ASN.1 string handling, delta CRL iteration fix, cache handling for missing nextUpdate).
- Add/clarify security warnings and documentation around ICV/DV derivation availability and unauthenticated HTTPS fetching.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util/icvutil.c | Clarifies runtime warning when UTA support is not compiled in. |
| src/libsecutils/src/util/util.c | Fixes UTIL_safe_string_copy() truncation handling and hardens UTIL_url_encode() indexing. |
| src/libsecutils/src/storage/files_icv.c | Removes redundant conditional around .cnf suffix handling. |
| src/libsecutils/src/storage/files_dv.c | Avoids logging DV-derived passwords; adds bounds check in path canonicalization. |
| src/libsecutils/src/crypto/crypto.c | Adds size/overflow checks in AES-GCM encrypt/decrypt paths. |
| src/libsecutils/src/credentials/store.c | Fails early when ctx is provided but ICV support is not compiled in. |
| src/libsecutils/src/connections/tls.c | Fixes potential double-free by nulling transferred BIO ownership. |
| src/libsecutils/src/connections/http.c | Adds explicit warning when using unauthenticated/trivial TLS for https downloads. |
| src/libsecutils/src/config/config.c | Makes config load fail when ctx is provided but ICV support is not compiled in. |
| src/libsecutils/src/certstatus/ocsp.c | Gates nonce usage by build flag and fixes error handling for nonce/cert-ID mismatch. |
| src/libsecutils/src/certstatus/crls.c | Hardens CDP URI extraction and fixes delta-CRL iteration source stack. |
| src/libsecutils/src/certstatus/crl_mgmt.c | Handles NULL nextUpdate; validates ASN.1 string data; caches only CRLs with nextUpdate. |
| src/libsecutils/src/certstatus/cdp_util.c | Hardens URI extraction against invalid/embedded-NUL ASN.1 strings. |
| src/libsecutils/include/secutils/util/util.h | Updates UTIL_safe_string_copy() API documentation. |
| src/libsecutils/include/secutils/storage/files_icv.h | Documents when secure ICV derivation is unavailable without UTA support. |
| src/libsecutils/include/secutils/storage/files_dv.h | Documents when secure DV-derived password is unavailable without UTA support. |
| src/libsecutils/include/secutils/operators.h | Updates author metadata. |
| src/libsecutils/include/secutils/crypto/crypto.h | Clarifies AES-GCM finalization docs and tag length expectations. |
| src/libsecutils/include/secutils/credentials/verify.h | Documents that “not yet valid” certificates are treated as warnings in verification callbacks. |
| src/libsecutils/include/secutils/credentials/trusted.h | Documents when ICV-based checks are skipped due to ctx/build settings. |
| src/libsecutils/include/secutils/credentials/store.h | Documents unlimited URL use from CDP/AIA and “not yet valid” handling; clarifies ICV behavior. |
| src/libsecutils/include/secutils/credentials/credentials.h | Clarifies DV-based protection behavior and corrects CREDENTIALS_get/store documentation. |
| src/libsecutils/include/secutils/connections/http.h | Documents trivial unauthenticated TLS behavior for https URLs. |
| src/libsecutils/include/secutils/certstatus/ocsp.h | Documents trivial TLS usage for https, nonce behavior, and URL access implications. |
| src/libsecutils/include/secutils/certstatus/crls.h | Documents trivial TLS usage for https and URL access implications. |
| src/libsecutils/include/secutils/certstatus/crl_mgmt.h | Updates author metadata removal. |
| src/libsecutils/include/secutils/certstatus/cdp_util.h | Updates author metadata removal. |
| README.md | Updates documented tested tool versions. |
| CMakeLists.txt | Adds pkg-config file cleanup to clean_all. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…these do *not* use DV-based protection
…SE_ICV not defined
…n ctx != NULL with SECUTILS_USE_ICV not defined
…= NULL or SECUTILS_USE_ICV not defined
… EVP_GCM_TLS_TAG_LEN
…h-bit char input bytes
…ned into just a warning This effectively accepts not yet valid certificates, which are considered OK here.
… TLS (via https URLs)
…e use trivial unauthenticated TLS
…n iterating over delta CRLs
…ove debugging output
…CDP/AIA entries is not limited
…long to fit destination buffer
…g. containing NUL byte
…ssibly being NULL
DDvO
force-pushed
the
fix_issues_by_ChatGPT
branch
from
d927dea to
1b17bbf
Compare
Member
Author
|
CI failure is unrelated: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ocsp.c:get_ocsp_resp(): fix error handling on missing/wrong nonce or cert ID,disable use of nonces unless defined
SECUTILS_OCSP_USE_NONCETLS_connect(): fix double free of connection BIOAESGCM_encrypt(),AESGCM_decrypt(): add missing checks for output buffer size and integer overflowscrypto.h: state that forAESGCM_{en,de}crypt_final(),tag_lenshould beEVP_GCM_TLS_TAG_LENfiles_dv.c: add missing buffer size info and bounds check toget_canonicalized()credentials.h,files_dv.h: point out when no secure password cannot be derived from DVfiles_dv.c getBase64Password(): do not trace-log DV-based passwordcredentials.h: fix doc ofCREDENTIALS_get()andCREDENTIALS_store(): these do not use DV-based protectionCONF_load_config(),STORE_load_more_check()andSTORE_load_crl_dir()fail onctx != NULLwithSECUTILS_USE_ICVnot definedstore.h,trusted.h: point out that no ICV-based check is done ifctx == NULLorSECUTILS_USE_ICVnot definedfiles_icv.{c,h},icvutil.c:: point out when no secure ICV can be derivedUTIL_url_encode(): prevent out-of-array access on char lookup for high-bit char input bytesverify.h,store.h: point out thatX509_V_ERR_CERT_NOT_YET_VALIDis turned into just a warningCONN_load_ASN1_http(): add warning when using trivial unauthenticated TLS (via https URLs)CONN_load_{ASN1,crl,OCSP}_http(): add note that with https URLs, these use trivial unauthenticated TLScrls.c check_cert_status_cdps(): fix wrongly using base CRL entry when iterating over delta CRLsstore.h,crls.h,oscp.h: add note that the use of URLs taken from cert CDP/AIA entries is not limitedcrl_mgmt.c get_crl_from_cache(): add missing checks for nextUpdate possibly being NULLcrl_mgmt.c get_crl_by_download_or_from_cache(): cache only CRLs having nextUpdateUTIL_safe_string_copy(): fix handling of the case that source is too long to fit destination bufferASN1_STRING_get0_data(): check for invalid string data, e.g. containing NUL byteIn addition:
files_icv.c: remove redundant inner#ifdef SECUTILS_USE_ICVcrls.c check_cert_status_cdps(): refactor introducing n_base and improve debugging outputcredentials.h: improve description of which kind of protection can be providedcrypto.h: fixAESGCM_encrypt_final()doc nitsicvutil.c,operators.h,cdp_util.{c,h},crl_mgmt.{c,h},crypto.{c,,h}: make author info more correctREADME.md: update list of tested (build) tool versionsCMakeLists.txt: add missing clean_all item (pkgconfig file{PROJECT_NAME}.pc)