RE: Bug in lib_ra_serf ssl_server_cert()

The problem here is two folded:
I agree that we *should* properly remove these items from the hash, but we have had the current behavior since 1.0 even in our previous dav implementation.

But given this and our documentation, the only thing about the passed hash that we promise are that for each callback type only specific keys are valid in the hashtable. All the other values are undefined and it is really upt o the api user on how that affects them. In general it should only use the defined values and ignore the rest.

If you are reading those other pointer values, there is no way you could know what they point to (They are just void*) and are undefined/may change. Your implementation should be careful not to just resolve these value and use the values for something. (We are free to store (void*)1 as marker or use undefined structure types, as we do in many other places).

I’m happy to apply a patch that fixes the first problem, but I don’t see a valid way to crash a valid usage of this api. Causing a segfault based on this problem relies on using undefined behavior somewhere.
(That’s why I didn’t direcly fix it when I noticed this same problem a few years ago... That and that I couldn’t think of a simple patch to easily fix this. Things might be easier today after recent refactorings)

   Bert

From: Simon Wilson
Sent: vrijdag 18 september 2015 15:05
To: dev_at_subversion.apache.org
Subject: Bug in lib_ra_serf ssl_server_cert()

We have found a bug in the ssl_server_cert() function in lib_ra_serf. This bug is present in 1.8.14 but we believe it has been present for some time.

The bug is as follows:

If Serf determines that a server certificate is invalid it calls into lib_ra_serf, which then requests two types of credentials from the authentication stack: SVN_AUTH_CRED_SSL_SERVER_AUTHORITY (this appears to be only relevant on Windows) and SVN_AUTH_CRED_SSL_SERVER_TRUST.

Calls to svn_auth_first_credentials(), svn_auth_next_credentials() and svn_auth_save_credentials() are preceded by calls to svn_auth_set_parameter() to set values for the SVN_AUTH_PARAM_SSL_SERVER_FAILURES and SVN_AUTH_PARAM_SSL_SERVER_CERT_INFO parameters (lines 371, 375, 418 and 422). Significantly, both of these parameters are set to the address of stack variables.

Both parameters are reset (i.e. removed) after the call to svn_auth_first_credentials() for SVN_AUTH_CRED_SSL_SERVER_AUTHORITY (lines 388 and 391). However, SVN_AUTH_PARAM_SSL_SERVER_FAILURES is *not* removed after requesting credentials of type SVN_AUTH_CRED_SSL_SERVER_TRUST (line 428) even though SVN_AUTH_PARAM_SSL_SERVER_CERT_INFO is (line 452).

As a result, the auth_baton’s parameter set is left with a value that points to a stack variable when the function ends. If this value is dereferenced at a later time (for example when responding to a request for another credential type such as SVN_AUTH_CRED_SIMPLE) then either the address is invalid and the application segfaults or garbage is read from the stack.

Proposed solution:

SVN_AUTH_PARAM_SSL_SERVER_FAILURES should be removed from the auth_baton’s parameters before ssl_server_cert() returns. This is already the case for SVN_AUTH_PARAM_SSL_SERVER_CERT_INFO.

As an aside:

The practice of temporarily setting pointers to stack variables as the values of auth_baton parameters seems questionable. This issue would have resulted in a benign, out-of-date parameter if the value for SVN_AUTH_PARAM_SSL_SERVER_FAILURES had been allocated from the appropriate pool rather than the stack.

Also:

One might question why an application needs to inspect the value of SVN_AUTH_PARAM_SSL_SERVER_FAILURES outside of the scope of a request for SVN_AUTH_CRED_SSL_SERVER_AUTHORITY or SVN_AUTH_CRED_SSL_SERVER_TRUST credentials.

In our case, the application is written in a higher-level language than C (Objective-C) and calls to our authentication providers pass through a C -> Obj-C bridge layer. As a result, the sets of parameters passed to our callback functions are converted into Obj-C dictionaries in their entirety. It was this conversion of invalid SVN_AUTH_PARAM_SSL_SERVER_FAILURES values that caused segfaults in our application.

Best regards,
Simon Wilson
Received on 2015-09-18 15:31:32 CEST