Repository navigation
check request common name in cupsSignCredentialsRequest - #189
tanjiroK-coder wants to merge 2 commits into
Conversation
michaelrsweet
left a comment
michaelrsweet
left a comment
There was a problem hiding this comment.
See comments; looks reasonable but I just want to confirm that the ASN1 stuff with OpenSSL is giving us a nul-terminated string...
| const ASN1_STRING *cn = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(subject, i)); | ||
| // commonName value | ||
|
|
||
| if ((size_t)ASN1_STRING_length(cn) != strlen(common_name) || _cups_strncasecmp((const char *)ASN1_STRING_get0_data(cn), common_name, strlen(common_name))) |
There was a problem hiding this comment.
Just to confirm - we know that ASN1_STRING_get0_data returns a nul-terminated ASCII string?
There was a problem hiding this comment.
It doesn't need to be. In practice OpenSSL does nul-terminate it (ASN1_STRING_set allocates len + 1 and writes a trailing nul, and the d2i path goes through that), but the docs don't promise it and a CN can carry embedded nuls, so the check doesn't rely on it: ASN1_STRING_length has to equal strlen(common_name) first, and the compare is bounded by that length, so it never reads past the encoded data. Something like CN=altprinter\0evil fails on the length mismatch.
Pushed a small tweak that keeps the length in cnlen and uses it as the compare bound with a comment saying why. Same behaviour, just more obvious.
| - Fixed a potential string overrun when loading .strings files (Issue #171) | ||
| - Fixed `cupsSignCredentialsRequest` to reject requests with a bad signature | ||
| (Issue 172) | ||
| - Fixed `cupsSignCredentialsRequest` to reject requests whose subject common |
There was a problem hiding this comment.
In the future, please just put your proposed changelog in the PR description and we'll reword and add the GitHub issue number for tracking later...
There was a problem hiding this comment.
Noted. I've dropped the CHANGES.md hunk from this PR and put the proposed entry in the description instead.
cupsSignCredentialsRequestis given the common name the caller has approved and checks every subjectAltName DNS value in the request against it through the SAN callback, but the request's subject DN is copied into the new certificate without its commonName ever being looked at. A device whose CSR gets signed by the site CA (for example withcups-x509 ca NAME) can list its own name as the only SAN and put any other host in the CN. With OpenSSL the resulting certificate is accepted for the CN host becauseopenssl_validate_namecompares the subject CN before the SAN list, and with GnuTLS the signed certificate carries no extensions at all, sognutls_x509_crt_check_hostnamefalls back to the CN. Either waycupsGetCredentialsTrustwithrequire_careturnsHTTP_TRUST_OKfor the impersonated host, which is whathttpConnectURIand the OAuth code rely on. I confirmed it by signing a request forCN=authserver.example.com, DNS:altprinteras "altprinter" against a test site CA:cupsAreCredentialsValidForName("authserver.example.com", ...)was true and the trust check returned OK. The fix rejects a request whose commonName values do not all match the common name being signed, in both backends, the same way a bad subjectAltName is rejected. The new testcreds case uses an embedded CSR (CN=badprinter, DNS:altprinter) that both backends sign on master and refuse with this change, andmake testpasses with OpenSSL 4.0.3 and GnuTLS 3.8.13. The new error message is not in the .strings files yet.Proposed changelog entry:
cupsSignCredentialsRequestto reject requests whose subject common name does not match the common name being signed.