Re: Bug in lib_ra_serf ssl_server_cert()

Hi Bert,

> 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.

This is true to a point. If lib_ra_serf was writing the parameters to an apr_hash_t that was private to the call stack (and therefore specific to the calling context) then I would concur.

However, lib_ra_serf uses svn_auth_set_parameter() to set *global* parameters for the auth_baton. As such, these parameters are published to the entire application: any other code might reasonably expect to be able to read these values (e.g. for debugging/diagnostics) at any time without crashing due to invalid pointers. They are also left as garbage in plain sight for any subsequent callback to any auth provider. This is just bad hygiene — lib_ra_serf should be able to clean up after itself!

> 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.

We’re not trying to extract the values for all the parameters in the hash, just the standard set of parameters defined in svn_auth.h (where the types are clearly defined). The rest are indeed ignored.

We have implemented two workarounds at our end: as a short-term fix we will only extract SVN_AUTH_PARAM_SSL_SERVER_FAILURES for callbacks requesting SVN_AUTH_CRED_SSL_SERVER_TRUST creds. We have also implemented a long-term fix where we extract properties from the hash upon request from the auth provider rather than converting all parameters up front.

I posted about the fix as a courtesy to the svn devs. It would be great if you could fix it, but we’re not dependent on it at our end.

Many thanks and best regards,
Simon

> On 18 Sep 2015, at 15:31 pm, bert_at_qqmail.nl wrote:
>
> 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