559769 - libpkix should allow the content-type application/x-pkcs7-crl when downloading a CRL over HTTP

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Attachment: Patch for pkix_pl_httpcertstore.c

libpkix insists on the content type application/pkix-crl when
downloading a CRL over HTTP:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_httpcertstore.c&rev=1.14&mark=478,483-484#478

I propose that libpkix also allow the content type
application/x-pkcs7-crl for two reasons:

1. Some CAs still use application/x-pkcs7-crl.  For example,
visit https://blackboard.mines.edu and
https://www.networksolutions.com/ and check the CRL distribution
points extensions of the those certificates.

2. RFC 3280 does not specify a content type for CRL.  Although
application/pkix-crl is specified in RFC 5280, it is just a
"SHOULD".  Section 4.2.1.13 "CRL Distribution Points" on page 46:

   ...  HTTP server implementations accessed via the URI SHOULD
   specify the media type application/pkix-crl in the content-type
   header field of the response.

Please review the attached patch.  Let me know if you think
PKIX_CONTENTTYPENOTPKIXCRL should be renamed
PKIX_CONTENTTYPENOTCRL.

Comment 1

[Robert Relyea]

Comment on attachment 439479 [details] [diff] [review]
Patch for pkix_pl_httpcertstore.c


sr+ rrelyea

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Yeah, if we ask for a CRL, and we get a reply that parses correctly as a 
DER CRL and has a valid signature, I'm not sure we care about its MIME 
Content-type string value.

Comment 3

[Robert Relyea]

Good point. We should handle the case that the CA gives us garbage even if the mime/type is set correctly. The mime-type is more for a browser to figure out where to route the request (I believe FF will import the crl if it has the correct mime-time).

bob

Comment 4

[Wan-Teh Chang]

Attachment: Patch for pkix_pl_pk11certstore.c

Bob, Nelson: thanks for the comments.  I have new findings about this bug.

I found that libpkix does NOT use pkix_pl_httpcertstore.c to download CRLs.
So my proposed patch (attachment 439479 [details] [diff] [review]) may be patching dead code.

Instead, libpkix uses pkix_pl_pk11certstore.c (the DownloadCrl function)
to download CRLs.  That function does NOT check the HTTP response
content-type at all!

So my question for you is: should I change pkix_pl_httpcertstore.c (the
dead code) to also ignore the HTTP response content-type, or should I
change pkix_pl_pk11certstore.c to check the content-type?  This patch
does the latter.

My preference is to ignore the HTTP response content-type of downloaded
CRLs, but it's not a strong preference.

Comment 5

[Wan-Teh Chang]

Attachment: Remove unnecessary typecasts

These typecasts are not necessary because the variables are already of
that type (const char **).

Comment 6

[Robert Relyea]

I too, think we can ignore the content-type. If we asked for a CRL and got something else, we'll know when we try to decode it. content-type is useful for things like browsers who just get a url and have no idea what it is they are pulling over.

bob

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

How about this compromise: scan the content-type case-insensitively for crl.
If you find it, the content type is close enough, else it's not.  
?

Comment 8

[Wan-Teh Chang]

PSM allows three content-types for CRLs:
http://mxr.mozilla.org/mozilla-central/search?string=pkix-crl

I'll remove the content-type check, unless they tell us whether
the CRL is encoded in binary (DER) or ASCII (PEM).

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, 
your mxr link appears to show that PSM allows only one content-type,
which is "application/pkix-crl".

I think it is useful to do SOME check on the content type, to avoid wasting
time processing huge gifs or tiffs and the like.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 440924 [details] [diff] [review]
Patch for pkix_pl_pk11certstore.c

looks OK to me. r+

Comment 11

[Wan-Teh Chang]

(In reply to comment #9)
No, PSM allows three content types.  Here are the excerpts of PSM code:

nsNSSComponent.cpp:

3350   else if (!nsCRT::strcasecmp(aContentType, "application/x-pkcs7-crl"))
3351     return PSMContentDownloader::PKCS7_CRL;
3352   else if (!nsCRT::strcasecmp(aContentType, "application/x-x509-crl"))
3353     return PSMContentDownloader::PKCS7_CRL;
3354   else if (!nsCRT::strcasecmp(aContentType, "application/pkix-crl"))
3355     return PSMContentDownloader::PKCS7_CRL;

nsNSSModule.cpp:

258   catman->AddCategoryEntry(
259     NS_CONTENT_LISTENER_CATEGORYMANAGER_ENTRY,
260     "application/x-pkcs7-crl",
261     info->mContractID, PR_TRUE, PR_TRUE, getter_Copies(previous));
262 
263   catman->AddCategoryEntry(
264     NS_CONTENT_LISTENER_CATEGORYMANAGER_ENTRY,
265     "application/x-x509-crl",
266     info->mContractID, PR_TRUE, PR_TRUE, getter_Copies(previous));
267 
268   catman->AddCategoryEntry(
269     NS_CONTENT_LISTENER_CATEGORYMANAGER_ENTRY,
270     "application/pkix-crl",
271     info->mContractID, PR_TRUE, PR_TRUE, getter_Copies(previous));

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, the point of my comment 9 was not to say that PSM sources only 
allow one content type, but rather that the URL that you provided in 
your comment 8 for the purpose of showing 3 content types actually only 
shows one content type.

Comment 13

[Robert Relyea]

Comment on attachment 440924 [details] [diff] [review]
Patch for pkix_pl_pk11certstore.c

r+ I would be ok with allowing x-x509-crl as well.

Comment 14

[Wan-Teh Chang]

Comment on attachment 440924 [details] [diff] [review]
Patch for pkix_pl_pk11certstore.c

Although I like to be strict in validating input,
I'm afraid that I have to abandon this patch.

Today I downloaded a CRL in the DER binary format
but the server sent the incorrect content-type
response header of text/plain.  The download URL is
http://www.public-trust.com/cgi-bin/CRL/2018/cdp.crl

It would be a shame if we discarded a CRL in the
correct DER format because the content-type is wrong.
I think we can only log an error message.  Sigh.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, 
I disagree.  Let's be strict.  Make the bozos fix their server.
At some point down the road, there may arise a different new CRL 
format, at which point the content type really will matter.  
That won't be a problem for us if we've been strict all along, 
but could be a huge problem for us if we've been allowing servers 
to use everything under the sun up until then.

Comment 16

[Jonathan Watt [:jwatt]]

I'm searching for patches with checkin-needed to check in at the same time as my own. This bug should not have that keyword set since it doesn't have approval, and so the keyword is just creating noise. Please remove it and - assuming it's there to keep this on someone's radar - use your own flags for that purpose in the whiteboard.

Comment 17

[Jonathan Watt [:jwatt]]

Sorry, ignore that, my search criteria was messed up.

Comment 18

[Kai Engert (:KaiE:)]

+1 for requiring a correct mimetype.

Can this be checked in?
What's left to be done?

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Which CAs in our root program (at a minimum) are affected by this change? Are they all using the correct content-type for their CRLs?

Comment 20

[Jakob Bohm]

I recommend that the patch to *add* mime checking is abandoned as a a very bad idea, and certainly not something to do under this bug number.

This bug, from its initial description and comments is a request to *not* reject CRLs just because they have a mime type other than the two-lately-registered "application/pkix-crl", which turns out to be already implemented except in a piece of dead code which should have been commented out to avoid confusion.

As I read comment 4, this bug is simply non-existing and should be closed as "confirmed not there" or a patch should be written to delete the dead buggy function in pkix_pl_httpcertstore.c, to prevent accidental use.

As for the question in comment 8: All the known crl mime types are defined to apply equally to the BER and PEM base64 forms.  There are several bug numbers already assigned to the lack of Base64 CRL acceptance in Mozilla.

As for the "huge gifs" issue mentioned in comment 9, CRL files have recognizable initial byte patterns, so NSS could/should be able to protect itself from accidentally wasting time parsing arbitrary noise.

If someone wants to insist on limiting the acceptable mime types, this should be a different bug number with a different title, but it seems that the mime types encountered in practice are: 1. The recently registered application/pkix-crl 2. The equivalent application/x-pkcs7-crl used by CAs that want compatibility with older clients, such as Firefox 1.0.  3. The equivalent application/x-x509-crl used for the same purpose and a different subset of older clients.  4. The web server default fallback types application/octet-stream and/or text/plain returned by some misconfigured servers (including many in-house CAs that are seen by Mozilla installations in those companies).

Comment 21

[Kaspar]

(In reply to Jakob Bohm from comment #20)
> As for the question in comment 8: All the known crl mime types are defined
> to apply equally to the BER and PEM base64 forms.

s/BER/DER/

> There are several bug numbers already assigned to the lack of Base64 CRL
> acceptance in Mozilla.

In May 2008, RFC 5280 was published with the following requirement in section 4.2.1.13:

   If the DistributionPointName contains a general name of type URI, the
   following semantics MUST be assumed: the URI is a pointer to the
   current CRL for the associated reasons and will be issued by the
   associated cRLIssuer.  When the HTTP or FTP URI scheme is used, the
   URI MUST point to a single DER encoded CRL as specified in
   [RFC2585].

Still not providing CRLs at URIs referenced in the CRLDistributionPoints extension in DER format is definitely a bad idea as of 2013.

> the mime types encountered in practice are: 1. The recently registered
> application/pkix-crl

Depends on your understanding of "recently": 

> Network Working Group                                        R. Housley
> Request for Comments: 2585                                       SPYRUS
> Category: Standards Track                                    P. Hoffman
>                                                                     IMC
>                                                                May 1999
[...]
> 4.2. application/pkix-crl
> 
>    To: [e-mail]
>    Subject: Registration of MIME media type application/pkix-crl
> 
>    MIME media type name: application
> 
>    MIME subtype name: pkix-crl

I don't have strong feelings about the "proper" solution for this bug, but configuring a CRLDP to serve its content with text/plain speaks volumes for the level of quality of the respective CA (it should be noted that Verizon has at least managed to change the content type of the CRL mentioned in comment 14 to application/x-pkcs7-crl in the meantime - the "home page" at http://www.public-trust.com/ still looks a bit odd, though).

Comment 22

[Jakob Bohm]

(In reply to Kaspar Brand from comment #21)

>> As for the question in comment 8: All the known crl mime types are defined
>> to apply equally to the BER and PEM base64 forms.
> 
> s/BER/DER/

No, I actually meant BER, in accordance with the Postel principle and the
historic lack of a specification (that I know of) restricting CRLs to DER
encoding.

> ...
> Depends on your understanding of "recently": 

The modern CA system was established in the early 1990s.  For security and
stability reasons, any change in processes such as CRL publication may/should
entail extensive audit oversight that no malicious changes are made to the
systems at the same time.  If an HSM is involved, such changes may be outright
impossible.  So any CA whose security critical processes were initiated before
May 2008 may be practically restrained from complying with any new limitations
introduced by RFC5280 (Noone mentioned RFC2585 here before you did, and I have
not yet checked which requirements were included there).

Also, there is an ongoing absolute necessity to maintain compatibility with
the millions (if not billions) of clients that were distributed in the past
and changing to the 1999 mime type may prevent usage by older client software.

Now why would anyone use such old client software to check a CRL?, surely
there would be security holes in such old software you may ask.  There can
be many reasons, but here is one real world scenario I have encountered more
than once:

When recovering from a complete system compromise the standard security
practice is to disconnect from all networks, install from known good physical
read only media, establish ultra restrictive firewall rules, connect to the
network temporarily, then install security updates via the most secure
downloads available, carefully checking signatures etc. on the way, until the
system has been gradually brought up to date and data can be restored.

Now due to various forms of Internet software delivery, the only read only
media to install from as step 1 (and thus the software which will be doing
the first CRL checks) are likely to be one of the following old factory
pressed physical CD-ROMs (not CD-Rs):
 - Microsoft Windows 2000 or older, compiled December 1999, feature freeze
earlier than that (Windows XP and later require internet activation).
 - Red Hat or Walnut creek Linux or BSD CD-ROMs, also from the 1990s (Around
2000, the practice of distributing Linux on pressed CD-ROMs from reputable
vendors faded in favor of online downloads, which need something to bootstrap
from).
 - Paid Netscape Navigator CD-ROMs, from before they were forced to give it
away as a free download.

> I don't have strong feelings about the "proper" solution for this bug, but
> configuring a CRLDP to serve its content with text/plain speaks volumes for
> the level of quality of the respective CA

Aside from the observed fluke, the most common case to consider is the
in-house CA run by a company or campus and trusted only by local
computers.  These validate identities by first hand knowledge and
generally function as just another minor system in the IT department,
rather than as a dedicated specialist company.  Hence lower standards of
technical skill may be expected and failing to configure a non-public web
server with the right mime-type for the single CRL file that gets uploaded
from the CA once in a while would be completely normal.

Comment 23

[Kaspar]

(In reply to Jakob Bohm from comment #22)
> (In reply to Kaspar Brand from comment #21)
> 
> >> As for the question in comment 8: All the known crl mime types are defined
> >> to apply equally to the BER and PEM base64 forms.
> > 
> > s/BER/DER/
> 
> No, I actually meant BER, in accordance with the Postel principle and the
> historic lack of a specification (that I know of) restricting CRLs to DER
> encoding.

You might want to consult CCITT X.509 Recommendation X.509 (11/1988, aka X.509 v1), where X.509 CRLs were specified for the first time:

> CertificateRevocationList ::= ATTRIBUTE
>   WITH ATTRIBUTE-SYNTAX CertificateList
> AuthorityRevocationList ::= ATTRIBUTE
>   WITH ATTRIBUTE-SYNTAX CertificateList
> CertificateList ::= SIGNED SEQUENCE{
>   signature AlgorithmIdentifier,
>   issuer Name,
>   lastUpdate UTCTime,
>   revokedCertificates
>     SIGNED SEQUENCE OF SEQUENCE{
>     signature AlgorithmIdentifier,
>     issuer Name, CertificateSerialNumber subject,
>     revocationDate UTCTime}
>       OPTIONAL}

(on p.19), and then on p.14

> 8.7 In order to enable the validation of SIGNED and SIGNATURE types in a
> distributed environment, a distinguished encoding is required. A distinguished
> encoding of a SIGNED or SIGNATURE data value shall be obtained by applying
> the Basic Encoding Rules defined in Recommendation X.209 with the following
> restrictions:
> a) the definite form of length encoding shall be used, encoded in the minimum
>    number of octets;
> b) for string types, the constructed form of encoding shall not be used;
> c) if the value of a type is its default value, it shall be absent;
> d) the components of a Set type shall be encoded in ascending order of their
>    tag value;
> e) the components of a Set-of type shall be encoded in ascending order of
>    their octet value;
> f) if the value of a Boolean type is true, the encoding shall have its
>    contents octet set to 'FF'16 ;
> g) each unused bits in the final octet of the encoding of a BitString value,
>    if there are any, shall be set to zero;
> h) the encoding of a Real type shall be such that bases 8, 10 and 16 shall
>    not be used, and the binary scaling factor shall be zero.

Unless you read "a distinguished encoding is required" to apply exclusively to
the signature validation process, and insist on CRL checking implementations having to fully parse and DER re-encode a CRL for validation purposes (which would be a very pecular interpretation), BER-encoded CRLs are, and have always been, nonsense.

Comment 24

[Jakob Bohm]

> Unless you read "a distinguished encoding is required" to apply exclusively to
> the signature validation process, and insist on CRL checking implementations
> having to fully parse and DER re-encode a CRL for validation purposes (which
> would be a very pecular interpretation), BER-encoded CRLs are, and have always
> been, nonsense.

There are 2 levels at which BER encoded CRLs could be recognized, In principle, I am only suggesting the first, simpler level.

Unsigned part only: In a signed CRL, parts of the encoded data is outside the part subjected to the hashing in the signature check (most notably the signature itself).  Those have no need to be in DER form and no need to be recoded to DER form for verification.  Postel principle should definitely apply here.

All parts: Just in case someone does this, it makes sense to do on-the-fly conversion of signed BER parts to their DER equivalent.  With properly written libraries, this can be done generically almost-on-the-fly without an error prone full decoding and reencoding.  Basically what is needed is a canonicalization function which does exactly what the quoted section 8.7 of the spec says: Change indefinite length and constructed lengths to definite length, remove default values, sort the elements of sets, fix the encoding of true booleans, bit strings and numbers as specified.  In the course of doing this, it may be needed to back up and change length bytes after cleaning up the contents they cover, so this is a two-pass process not involving conversion to non-BER data types (for instance a GeneralTime element should not be converted to time_t and back).  It will be a lot like programming a modified two-tape Turing machine with a two-tape, with the added ability to let the pen trail a variable number of symbols behind the reader on the main tape.