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