subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Simon Wilson <si...@zennaware.com>
Subject Re: Bug in lib_ra_serf ssl_server_cert()
Date Fri, 18 Sep 2015 20:18:58 GMT
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@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@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


Mime
View raw message