Closed
Bug 1257031
Opened 8 years ago
Closed 8 years ago
Return more informative error code when encountering invalid integers rather than SEC_ERROR_BAD_DER
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: Cykesiopka, Assigned: Cykesiopka)
References
Details
(Whiteboard: [psm-assigned])
Attachments
(1 file)
Once in a while we get reports of people running into SEC_ERROR_BAD_DER errors because of improperly encoded integers (e.g. accidental negative serial numbers). The See Also field has some example bugs. It would be nice to introduce a new error code for improperly encoded integers so that it's clearer what the issue is, and to save some cert analysis. A new error code won't make a difference to an affected end-user (still no override allowed), but at least a specific error is easier to report to whoever generated the certificate in question.
Whiteboard: [psm-backlog]
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Whiteboard: [psm-backlog] → [psm-assigned]
Assignee | ||
Comment 1•8 years ago
|
||
Also adds some missing l10n entries to nsserrors.properties (but not for errors that are specific to TLS 1.3, since TLS 1.3 is not yet finalised). Review commit: https://reviewboard.mozilla.org/r/47607/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47607/
Attachment #8743163 -
Flags: review?(dkeeler)
Comment on attachment 8743163 [details] MozReview Request: Bug 1257031 - Return more informative error code when encountering invalid integers rather than SEC_ERROR_BAD_DER. r=keeler https://reviewboard.mozilla.org/r/47607/#review44563 LGTM. I bikeshedded slightly on the wording of the error message. ::: security/manager/locales/en-US/chrome/pipnss/nsserrors.properties:328 (Diff revision 1) > MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE=A certificate that is not yet valid was used to issue the server's certificate. > MOZILLA_PKIX_ERROR_SIGNATURE_ALGORITHM_MISMATCH=The signature algorithm in the signature field of the certificate does not match the algorithm in its signatureAlgorithm field. > MOZILLA_PKIX_ERROR_OCSP_RESPONSE_FOR_CERT_MISSING=The OCSP response does not include a status for the certificate being verified. > MOZILLA_PKIX_ERROR_VALIDITY_TOO_LONG=The server presented a certificate that is valid for too long. > MOZILLA_PKIX_ERROR_REQUIRED_TLS_FEATURE_MISSING=A required TLS feature is missing. > +MOZILLA_PKIX_ERROR_IMPROPERLY_ENCODED_INTEGER=The server presented a certificate that improperly encodes an integer. Common causes include negative serial numbers and encodings that are longer than necessary. I think "invalid" might have a more accurate connotation than "improper" in this case. Also, rather than saying the certificate encodes the integer, maybe say the certificate contains an invalid encoding of an integer or something. Also, might mention negative RSA modulii as well. ::: security/pkix/lib/pkixder.cpp:497 (Diff revision 1) > /*optional out*/ Input::size_type* significantBytes) > { > Result rv = ExpectTagAndGetValue(input, tag, value); > if (rv != Success) { > + if (rv == Result::ERROR_BAD_DER) { > + return Result::ERROR_IMPROPERLY_ENCODED_INTEGER; I'm not sure we want to convert bad DER to invalid integer here. I think if ExpectTagAndGetValue fails, we've either run out of input or we've encountered an unexpected tag, which is more a bad DER issue than an invalid integer issue.
Attachment #8743163 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 3•8 years ago
|
||
https://reviewboard.mozilla.org/r/47607/#review44563 Thanks! > I think "invalid" might have a more accurate connotation than "improper" in this case. Also, rather than saying the certificate encodes the integer, maybe say the certificate contains an invalid encoding of an integer or something. > Also, might mention negative RSA modulii as well. Sounds good. > I'm not sure we want to convert bad DER to invalid integer here. I think if ExpectTagAndGetValue fails, we've either run out of input or we've encountered an unexpected tag, which is more a bad DER issue than an invalid integer issue. Yes, good point.
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8743163 [details] MozReview Request: Bug 1257031 - Return more informative error code when encountering invalid integers rather than SEC_ERROR_BAD_DER. r=keeler Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47607/diff/1-2/
Attachment #8743163 -
Attachment description: MozReview Request: Bug 1257031 - Return more informative error code when encountering invalid integers rather than SEC_ERROR_BAD_DER. → MozReview Request: Bug 1257031 - Return more informative error code when encountering invalid integers rather than SEC_ERROR_BAD_DER. r=keeler
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ae3211493ae4e60dde6c897f948780719b0ad4e I cancelled the Win7 tests because backlog is apparently so ridiculous that the tests still didn't start 13+ hours later.
Keywords: checkin-needed
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/35edab8d84db
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 8•8 years ago
|
||
Just checking, is that part of the string correct? “RSA moduli”
Flags: needinfo?(dkeeler)
Flags: needinfo?(cykesiopka.bmo)
Yes - moduli is the plural of modulus. Overall the phrase refers to a component of an RSA public key that is sometimes incorrectly encoded.
Flags: needinfo?(dkeeler)
Flags: needinfo?(cykesiopka.bmo)
Comment 10•8 years ago
|
||
Ok, wasn’t sure, thanks for confirming
You need to log in
before you can comment on or make changes to this bug.
Description
•