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.
