Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fractional seconds in ocsp.CertStatus #185

Open
neildunbar opened this issue Jun 4, 2020 · 8 comments
Open

Fractional seconds in ocsp.CertStatus #185

neildunbar opened this issue Jun 4, 2020 · 8 comments

Comments

@neildunbar
Copy link

Because CertStatus can take a revocation time, when setting that, might it be an idea to zero any fractional seconds which might exist in the native datetime?

RFC 6960's entry on GeneralizedTime refers back to RFC 5280, Section 4.1.2.5.2, which states:

GeneralizedTime values MUST NOT include fractional seconds.

Of course, one can always just .replace(microsecond=0) on the datetime value one sets, but this might avoid the danger of creating OCSP Responses (and possibly CRLs, after 2050!) which violate the relevant RFCs.

@joernheissler
Copy link
Collaborator

Hello!
Could you please provide a code snippet that generates such a CertStatus with fractions?

@joernheissler
Copy link
Collaborator

Quote from X.690 (§11.7.3):

The fractional-seconds elements, if present, shall omit all trailing zeros; if the elements correspond to 0, they
shall be wholly omitted, and the decimal point element also shall be omitted.

So the GeneralizedTime type is allowed to include fractions. RFC5280 restricts it further. Full quote:

For the purposes of this profile, GeneralizedTime values MUST be
expressed in Greenwich Mean Time (Zulu) and MUST include seconds
(i.e., times are YYYYMMDDHHMMSSZ), even where the number of seconds
is zero. GeneralizedTime values MUST NOT include fractional seconds.

So in general it's fine to accept fractions. Only when creating X.509 objects, it's not.

@neildunbar
Copy link
Author

So a bit of code using ocspbuilder:

from datetime import datetime, timezone

from oscrypto import asymmetric

from ocspbuilder import OCSPResponseBuilder

privkey_bytes = b64decode(
    """
MIIBVgIBADANBgkqhkiG9w0BAQEFAASCAUAwggE8AgEAAkEA5hMXb2uO2TZ6BUUU
nHwA+7xVBw0ifF13bt339r9hLhJKnAGbqqxRF/PcSm3IlM14/j53T9KbZmxmd6JQ
GiLapwIDAQABAkAiKzGuzXWAktOaVsER4GSw/i5OhsfZWnQzVenOjmubULoGmPB9
L3Dz4UC1B/usKdbkHgH6moyOR9Oigyvp7MfJAiEA9Y11vPkWrHPqyBY1Dv2GgHag
9dpkAEmV2R5nVCiDj+UCIQDv3Qvhenut3UpETan9hp1LHrl2ZUt8+YfHcPyIBvYf
mwIhAL2YLfJtOW6KShuX2fvrEPEbp4hsyY3XQ1ZTPWEjrwFpAiEAlSPfGEKdFhzq
6Y9UrAOAV83xyTDwf/NzPkn9auLRNBMCIQC94SBmMuEjWz1x/EzGI7fBuRJ76iHH
hCSFyAAYLXlkJQ==
"""
)

issuer_bytes = b64decode(
    """
MIIBezCCASWgAwIBAgIUaYsvXH25fbTOhy0zqJ3XextaoYIwDQYJKoZIhvcNAQEL
BQAwEjEQMA4GA1UEAwwHRmFrZS1DQTAeFw0yMDA2MDQxMzA1MzNaFw0zMDAxMDIx
MzA1MzNaMBIxEDAOBgNVBAMMB0Zha2UtQ0EwXDANBgkqhkiG9w0BAQEFAANLADBI
AkEAq8d0vbR6Z3DrQRaDnPIRzAOXOCFSL5/F6Px/BJ2oFj9rxkwYrDvCKYp7vh+3
ctAVFBPo6IxLimjSPBQDKHWpgwIDAQABo1MwUTAdBgNVHQ4EFgQUKRRbFI+sgdMn
m4vpg+Z2wDXQz7cwHwYDVR0jBBgwFoAUKRRbFI+sgdMnm4vpg+Z2wDXQz7cwDwYD
VR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAANBAHhWtrHl2h2Y7TYl7jYJttOb
7Tww5WU5kiP2Rmxmle3B8nRSpmi14Bp/FmLhcLfbFG6N3HYWAuuysTKveIOgDCg=
"""
)

subject_bytes = b64decode(
    """
MIIBKDCB0wIUYBmxJxoPx5p9buqW1+mocngt9gswDQYJKoZIhvcNAQELBQAwEjEQ
MA4GA1UEAwwHRmFrZS1DQTAeFw0yMDA2MDQxMzI3MzNaFw0yMTA1MzAxMzI3MzNa
MBoxGDAWBgNVBAMMD0Zha2UtRW5kX0VudGl0eTBcMA0GCSqGSIb3DQEBAQUAA0sA
MEgCQQCnCXhR6CvQCv4tGRZ2onZ4GyUbI4Mlw8wcqSSANgYXAdtflycurrpxpA5R
PHUWIegKpOGfgiI1ffFpDqUvx1fZAgMBAAEwDQYJKoZIhvcNAQELBQADQQBROqMb
QPrrFN/dync8miHtUp5HSzMzxP1DVUysEwkPJoKQ7UoH3doje6aIwGuhsfbt+URe
9uTVSfVBja9WXaB2
"""
)


def main():
    issuer_key = asymmetric.load_private_key(privkey_bytes)
    issuer_cert = asymmetric.load_certificate(issuer_bytes)
    subject_cert = asymmetric.load_certificate(subject_bytes)
    dt = datetime(
        year=2019,
        month=12,
        day=25,
        hour=12,
        minute=30,
        second=6,
        microsecond=12345,
        tzinfo=timezone.utc,
    )
    builder = OCSPResponseBuilder("successful", subject_cert, "key_compromise", dt)
    ocsp_response = builder.build(issuer_key, issuer_cert)
    print(b64encode(ocsp_response.dump()).decode("ascii"))


if __name__ == "__main__":
    main()

Run that (say it's called frac-ocsp.py) as

python frac-ocsp.py | base64 -d | openssl ocsp -respin - -noverify -resp_text

and you get

OCSP Response Data:
OCSP Response Status: successful (0x0)
Response Type: Basic OCSP Response
Version: 1 (0x0)
Responder Id: 29145B148FAC81D3279B8BE983E676C035D0CFB7
Produced At: Jun 4 13:30:11.666238 2020 GMT
Responses:
Certificate ID:
Hash Algorithm: sha1
Issuer Name Hash: 1896673A3F00652F42E9713459F6C4FB712C708E
Issuer Key Hash: 29145B148FAC81D3279B8BE983E676C035D0CFB7
Serial Number: 6019B1271A0FC79A7D6EEA96D7E9A872782DF60B
Cert Status: revoked
Revocation Time: Dec 25 12:30:06.012345 2019 GMT
Revocation Reason: keyCompromise (0x1)
This Update: Jun 4 13:30:11.666238 2020 GMT
Next Update: Jun 11 13:30:11.666238 2020 GMT

Signature Algorithm: sha256WithRSAEncryption
     d9:bb:0e:3a:b0:c4:63:ba:57:dc:92:f9:d0:db:c0:f0:9d:d6:
     2f:69:02:e7:95:74:65:e3:3a:79:fb:b8:2f:26:71:04:8c:9b:
     f9:9a:88:bc:7a:41:2d:22:9e:d4:89:bb:ee:b3:e8:34:03:74:
     63:c2:74:2f:d3:84:17:fc:8c:bd

@neildunbar
Copy link
Author

So in general it's fine to accept fractions.

Correct. The GeneralizedTime object is 100% fine. It could and should accept fractional seconds.

Only when creating X.509 objects, it's not.

Yup; and since a CertStatus is part of a profile defined via RFC 6960 (via RFC 5280), fractional seconds are a bad thing. It's not a bug, it's just that if you don't take care to zero the microseconds before you build, you'll get a non-compliant object.

@neildunbar
Copy link
Author

neildunbar commented Jun 4, 2020

Also to point out: the produced_at, this_update and next_update shouldn't have fractional components either, but that's OCSP in general, not CertStatus.

@joernheissler
Copy link
Collaborator

There are several options here:

  • Do nothing. As you pointed out, this may result in incorrect encodings.
  • When encoding certain types (e.g. CertStatus), silently remove fractions.
  • Print a warning, but include the fractions.
  • Print a warning, but strip the fractions.
  • Raise an Exception.
  • Remove fraction support from GeneralizedTime.

I don't think fraction support should be removed, because it's valid and someone might have a legitimate reason to encode them.
There are many incorrectly encoded ASN.1 objects in the wild and asn1crypto tries to do better. It's not a fully-featured ASN.1 library that allows generating all sorts of wrong encodings.
For sake of user experience and generating correct output, best would be to strip fractions when encoding certain types.

Might be possible by adding a mixin similar like _ForceNullParameters to some classes.

@wbond your thoughts on this?

@neildunbar
Copy link
Author

There are several options here:

  • Do nothing. As you pointed out, this may result in incorrect encodings.

And that's fine - as long as the programmer knows it could create RFC violating objects (ie, just mention it in the docs)

  • When encoding certain types (e.g. CertStatus), silently remove fractions.

I think that's what I would do. See which types embed types and are created out of PKIX RFCs and strip fractional seconds for them. It would even be possible to have an override flag which allows the fractional seconds, if the programmer wanted that behaviour.

  • Print a warning, but include the fractions.
  • Print a warning, but strip the fractions.

Both useful.

  • Raise an Exception.

Definitely helps to educate the programmer, but isn't it just as easy to remove the fraction component?

  • Remove fraction support from GeneralizedTime.

I don't think this is a good approach either. GeneralizedTime isn't defined from the PKIX RFCs, so the ability to timestamp fractions is useful. I can see a high accuracy timestamping service based on X.500 objects needing such precision.

@wbond
Copy link
Owner

wbond commented Jul 27, 2020

It sounds like the actionable items from this are:

  • Create an extension of GeneralizedTime that removes fractional components
  • Have all usage in x509.py, ocsp.py and crl.py should use this variant

I haven't spent any time looking to see if the TSP or CMS usage should allow fractional components. I probably won't in the near term, but if one of you has guidance or knowledge about that, I'd be happy to work on getting it fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants