On 8/6/14 2:09 PM, Stefan Fuhrmann wrote:
> What would a worst-case failure scenario look like? Could a faulty
> parser result in the auth store reporting keys that the user does not
> want to trust (e.g. by stitching together random portions of the file)?
I'll go ahead and answer the question I think you're asking first and then I'll
talk about some other security related details that I'm not sure if you're
asking about or not.
Stitching together random bits of a certificate is not likely to result in
something that this parser would accept. It's not so much a generic ASN.1
parser that just happens to be parsing X.509 as it is an X.509 parser that has
some ASN.1 logic hard coded to be able to work. So if things are out of place
in the slightest bit it's going to fail.
Most of the tricky bits of this was figuring out how to properly deal with
character set encodings. There rare 5 character sets that get used in X.509:
UTF8String (we do nothing other than validate it's UTF-8)
BMPString (this is UCS-2, easy to convert to UTF-8)
UniversalString (this is UCS-4, also easy to convert to UTF-8)
IA5String (this is ASCII)
TeletexString/T61String (this is a mess, everyone just treats it as ISO-8859-1
even though it's not. We follow the norm for everyone else).
I suppose it's possible that we have a bug in the character set encodings and
that causes us to display data that's just wrong enough that the user doesn't
get the correct view. But I think this is not likely. Converting these
character sets is largely a matter of following rules, they're all subsets of
Unicode so we just have to alter the encoding not the code points being used.
I think if we had an error here it would be obvious.
The only other thing we represent is the dates. There are some possible
ambiguities in the date format but they are tested for and we've implemented
with the RFC says.
I'm not sure though what sort of scenario you're envisioning with this
question. The parser has nothing to do with the data that gets put into the
auth store (for now, more on that later) nor does it have anything to do with
deciding if the certificates are ok. It may have something to do with
prompting but I'll talk about that later.
There is a note in the documentation for the svn_x509_parse_cert() function
that says that this function doesn't validate the data it returns. This is
because the parser does not validate that the certificate is valid, it just
returns what's in it. We don't check to make sure the issuer signature is
correct or that the issuer is trusted. Part of the reason for this is we may
not even have the issuer certificate to do such checking, we only store the
server certificate, not the whole chain. Since we're not using OpenSSL we
don't necessarily have access to the same trust store as Serf would be using.
We also don't look for extensions that we don't know about that are marked with
the critical flag, which in general would mean we should error for the purpose
of validation if we don't know how to handle them.
None of this is the slightest bit unusual for a system displaying a certificate
to a user. The openssl x509 command does not do any of this when you ask it to
display a certificate. Serf may return an error to us saying the certificate
is untrusted for various reasons but will always return the parsed content of
the file which may indeed have a bogus Issuer Distinguished Name.
The only place we end up using the parser outside of handling the `svn auth`
command is in JavaHL which uses it to parse the cert in the case of needing to
prompt for trust because the existing callbacks (which fill in
svn_auth_ssl_server_cert_info_t) don't provide all the data our parser does
(which fills in svn_x509_certinfo_t) and Branko wanted to used the same class
for wrapping the return from our X.509 parser and when prompting for a cert, so
he used the raw cert provided by the callbacks to call our parser. The biggest
difference being that we only provide a single hostname which is whichever one
we decided happens to have matched (in the case of Subject Alt Names) or the
Common Name if none of them match.
I don't see this usage as risky from a security standpoint. JavaHL still
provides the certificate failures and the information we already provide the
callback is just as unreliable as the information from our parser. The only
risk I see here is if the X.509 parser has bugs in it with some edge cases in
the certs that I'm unaware of and didn't test then it's possible this might
break when used with a certificate we used to work with. But it should be
fairly trivial to fix since it just means fixing the parser. Doing this same
sort of thing to our command line client is probably a good idea because then
we can display all the hostnames the certificate can match when prompting for
trust. But I'd rather give the parser some real world exposure before
inserting it into this important of a code path.
Branko has suggested that we provide the svn_x509_certinfo_t value to all the
callback providers, which could have been done by changing this API but I
didn't feel it was worthwhile to change these APIs when I believe we'll want to
change them much more extensively. As things stand right now the current
design of the ssl server trust store is very bad in light of Subject Alternate
Names.
Certs are cached based on the auth realm including the hostname. This means
that for instance if you're using a server with a wildcard hostname that you're
going to end up storing the certs as many times as hostnames you end up using
(googlecode is good example of where this happens, since they have a wildcard
and every project gets it's own hostname). This behavior also makes things
ugly if we want to allow users to add certs via a command to be trusted. Since
they have to know the authentication realm to cache the cert from. An
alternative that solves both of these is to cache based on fingerprint of the
certificate. Which is something I'd like to pursue, possibly in 1.10.x.
We'll probably use the parser to display certs before adding them as trusted
via such a command. Partly because running the parser lets us see if the file
they're giving us at least looks like a certificate and partly to let them
confirm they're adding the cert they want. In that case I still don't think
these are huge issues because if they're adding a cert to trust it's not going
to be validating via a trust chain. So the user has to be validating the cert
some other way (hopefully via the fingerprint).
The only other possible risk is the whole buffer overflow situation. ASN.1 is
a length,tag,data format. So it's possible to send a length that is longer
than the data. The parser keeps track of the end based on the length of the
buffer it's told and if things don't match up it returns a
SVN_ERR_ASN1_LENGTH_MISMATCH error. So I believe this is handled.
So in summary, from a security perspective I don't think there is a worst case
scenario. From a trust perspective only thing that could really go wrong is if
we decode things wrong and they end up decided to trust something they wouldn't
in the JavaHL case or decide to leave a trust they'd previously allowed when
reviewing them in the svn auth case. I find that highly unlikely.
Received on 2014-08-07 18:42:33 CEST