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, b...@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
smime.p7s
Description: S/MIME cryptographic signature
