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)

defect
Not set
normal

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.
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Whiteboard: [psm-backlog] → [psm-assigned]
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+
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.
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
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
https://hg.mozilla.org/mozilla-central/rev/35edab8d84db
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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)
Ok, wasn’t sure, thanks for confirming
You need to log in before you can comment on or make changes to this bug.