User: Password:
|
|
Subscribe / Log in / New account

A longstanding GnuTLS certificate validation botch

A longstanding GnuTLS certificate validation botch

Posted Mar 5, 2014 21:53 UTC (Wed) by Karellen (subscriber, #67644)
In reply to: A longstanding GnuTLS certificate validation botch by zorro
Parent article: A longstanding GnuTLS certificate validation botch

As I read it, even with a language that supports them, these are not the sort of errors you would handle with exceptions.

The code is checking for various conditions, with the checks returning success or failure, and the code itself needing to propagate success or failure. Having a function called check_if_ca() throw an exception from the checks it's doing, would be like having a string comparison function that threw if the strings weren't equal, or a character classification function throwing if the character you passed it didn't fall into the right category. It's just not the way you ought to handle that sort of "error".

You can very well argue that having strcmp() return 0 for a match, and non-0 for failure, where isdigit() returns non-zero for a match and 0 for a failure, is confusing, inconsistent, and ought to be changed. You might even be right. But (IMNSHO) changing them to throw on failure would be an even bigger mistake.


(Log in to post comments)

A longstanding GnuTLS certificate validation botch

Posted Mar 6, 2014 0:38 UTC (Thu) by jwakely (guest, #60262) [Link]

> As I read it, even with a language that supports them, these are not the sort of errors you would handle with exceptions.

Maybe not, but destructors still make it easier to get cleanup correct, whether exiting a function by returning or by throwing an exception, and it's that kind of "must be performed before returning" cleanup that the "goto cleanup" and "goto fail" jumps are performing.

> Having a function called check_if_ca() throw an exception from the checks it's doing, would be like having a string comparison function that threw if the strings weren't equal, or a character classification function throwing if the character you passed it didn't fall into the right category. It's just not the way you ought to handle that sort of "error".

As you implied by putting "error" in quotes, neither the strcmp case nor isdigit case is an error at all.

> You can very well argue that having strcmp() return 0 for a match, and non-0 for failure,

The results "compares less than" and "compares greater than" are not "failure", they're two of the three valid results from strcmp. None of those results indicates an error. Similarly for isdigit, a false result is not an error.

> But (IMNSHO) changing them to throw on failure would be an even bigger mistake.

Well yes, obviously. An exception means something went wrong, not "the answer to your question is no". Using an exception to answer whether a character is a digit or not would be dumb. The result of strcmp is not "pass" or "fail" so throwing an exception makes no sense at all. Throwing an exception might make sense if the argument to isdigit is not representable as unsigned char, or if either argument to strcmp is null (and indeed C++ implementations are already allowed to do that because such arguments produce undefined behaviour).

A longstanding GnuTLS certificate validation botch

Posted Mar 6, 2014 7:03 UTC (Thu) by Karellen (subscriber, #67644) [Link]

Maybe not, but destructors still make it easier to get cleanup correct, whether exiting a function by returning or by throwing an exception, and it's that kind of "must be performed before returning" cleanup that the "goto cleanup" and "goto fail" jumps are performing.

You're absolutely right, and I totally agree... but I can't quite figure how that relevant here. AIUI the problem here has very little (if nothing) to do with cleanup, and is down to confusion over whether success is indicated with a return value of 0 or 1.

A longstanding GnuTLS certificate validation botch

Posted Mar 6, 2014 14:05 UTC (Thu) by jwakely (guest, #60262) [Link]

Is it really confusion about the correct value to return, or just failing to set the correct value in the relevant variable?

Part of the problem seems to be that the "goto cleanup" style requires a single exit from the function, so you have to be sure to set the correct return value before reaching that single exit point.

If you have destructors (or cleanup attributes, or whatever) that run automatically on exit from the function you just "return 0;" immediately as soon as you like, instead of setting a value, then hoping it doesn't get changed again before the end of the function, then running the cleanup code, and then returning that value.

A longstanding GnuTLS certificate validation botch

Posted Mar 6, 2014 14:34 UTC (Thu) by dlang (subscriber, #313) [Link]

> Is it really confusion about the correct value to return

Yes, there are multiple common standards

0 = success
!0 = success
>=0 = success

which one you want depends on how many variations of success and not success you have and want to differentiate between.

As I understand this bug, different parts of code used different standards, and some function did the equivalent of:
myfunction() {
return otherfunction()
}
when otherfunction used one success standard but the caller of myfunction expected a different standard

so otherfunction() thought it was reporting a failure, but the caller of myfunction interpreted it as success.

A longstanding GnuTLS certificate validation botch

Posted Mar 6, 2014 0:59 UTC (Thu) by mathstuf (subscriber, #69389) [Link]

If I were to write it in a safer language (say…Haskell), I'd have something like:

sslChecks :: [CertChain -> Cert -> Reader SslContext (Maybe String)]
sslChecks = [validCert, acceptableAlgorithm, trustedChain, ...]

sslCheck :: CertChain -> Cert -> Reader SslContext (Maybe String)
sslCheck chain cert = (liftM mconcat . sequence) . map ($ cert) . map ($ chain) $ sslChecks

where a failure just bails out of the code at the end (the Reader monad stores the options any checks might care about). No style, error checking, boilerplate, or whatever to worry about. Just write a check, put it in the right place in the lift of checks to make and return an error string if necessary.

A longstanding GnuTLS certificate validation botch

Posted Mar 6, 2014 1:08 UTC (Thu) by mathstuf (subscriber, #69389) [Link]

Bleh…and that's wrong since mconcat isn't set up for "Nothing" to be the success :/ . I guess adding an Monoid instance for Either a () would work better.

A longstanding GnuTLS certificate validation botch

Posted Mar 6, 2014 1:49 UTC (Thu) by nybble41 (subscriber, #55106) [Link]

That version of sslCheck will concatenate the error strings together end-to-end if multiple checks fail (Just "First ErrorSecond Error"). Did you intend something like this, which returns a list of errors?

sslCheck :: CertChain -> Cert -> Reader SslContext [String]
sslCheck chain cert = liftM catMaybes $ sequence $ sslChecks <*> pure chain <*> pure cert

A longstanding GnuTLS certificate validation botch

Posted Mar 6, 2014 2:00 UTC (Thu) by mathstuf (subscriber, #69389) [Link]

That looks much better. I haven't done much Applicative work (which is how I tried it first) and missed the 'pure'. The original idea was to get just the first error message, but why not all :) .

A longstanding GnuTLS certificate validation botch

Posted Mar 6, 2014 12:28 UTC (Thu) by dgm (subscriber, #49227) [Link]

> You can very well argue that having strcmp() return 0 for a match, and non-0 for failure, where isdigit() returns non-zero for a match and 0 for a failure, is confusing, inconsistent, and ought to be changed.

I see where are you coming from, but I cannot agree. strcmp() is not for checking equality, even if you can use it for that. It's an order operator, because the sign of the return value matters, so the return value cannot be a boolean.

In any case the _only_ way to remove error paths -either implicit or explicit- from your code is to create complete functions (http://en.wikipedia.org/wiki/Functional_completeness) whenever possible.

A longstanding GnuTLS certificate validation botch

Posted Mar 6, 2014 21:31 UTC (Thu) by iabervon (subscriber, #722) [Link]

This actually is a reasonable situation for a semi-unexpected exception. The function is supposed to check if a cert is a CA or not, and the bug happens when the cert is mangled. It's like having strcmp throw an exception if one of the strings isn't nul-terminated in its allocation. It's likely that the intended behavior of the function is to treat garbage as not a trusted CA cert, but getting an exception would be secure, if not necessarily convenient.


Copyright © 2017, Eklektix, Inc.
Comments and public postings are copyrighted by their creators.
Linux is a registered trademark of Linus Torvalds