> static dbus_bool_t > do_check_nonce (DBusSocket fd, const DBusString *nonce, DBusError *error) > { > DBusString buffer; > DBusString p; > size_t nleft; > dbus_bool_t result; > int n; > > _DBUS_ASSERT_ERROR_IS_CLEAR (error); > > nleft = 16; > > if ( !_dbus_string_init (&buffer) > || !_dbus_string_init (&p) ) { > dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL); > _dbus_string_free (&p); > _dbus_string_free (&buffer); > return FALSE; > } If we fail to initialize buffer, p has undefined contents and it is not valid to free it. This is undefined even if we make _dbus_string_init guarantee to blank its argument on failure, due to the use of the short-circuiting || operator. do_noncefile_create() has a more complicated form of the same bug.
Created attachment 135266 [details] [review] do_check_nonce: Don't free uninitialized memory on OOM If _dbus_string_init() fails, it doesn't guarantee that the string is initialized to anything in particular. Worse, if _dbus_string_init (&buffer) fails, p would never have been initialized at all, due to the use of the short-circuiting || operator. --- As with at least one of the commits in Bug #101354, this is using a non-obvious corner of the API: it is useless but valid to "free" a constant string, and initializing a DBusString to be constant can't fail. The long-term solution is to do Bug #89104 instead, but that's going to require some more disentangling assumptions, and isn't suitable for stable-branches. Proposed for 1.12 too.
Created attachment 135267 [details] [review] do_noncefile_create: Avoid freeing uninitialized memory on error We could free all of these without having ever successfully initialized them. --- Proposed for 1.12 too.
Created attachment 135268 [details] [review] _dbus_accept_with_noncefile: Don't leak nonce This was always leaked, both on success and on error.
Created attachment 135269 [details] [review] DBusNonceFile: Don't rely on caller preallocating the object If we combine the dbus_new0, populating the DBusString members and the actual creation of the file, RAII-style, then we never need to worry about a partially-initialized or uninitialized DBusNonceFile becoming visible to a caller. Similarly, if we combine deletion of the file, freeing of the DBusString members, freeing the structure and clearing the pointer to the structure, then we can never be in an inconsistent situation, except during the actual implementation of _dbus_noncefile_delete(). Note that there are two implementations each of _dbus_noncefile_create() and _dbus_noncefile_delete(). This is because on Unix we must use a subdirectory of _dbus_get_tmpdir() (the nonce filename is not created atomically, so that would not be safe), while on Windows we use the directory directly (the Windows temp directory is private to a user, so this is OK). --- This entire module is rather suspect, because it adds an additional layer of authentication over dbus' SASL. I think it might be there because _dbus_read_credentials_unix_socket() was implemented like this on Windows: > /* FIXME bogus testing credentials */ > _dbus_credentials_from_current_process (credentials); which made EXTERNAL authentication completely meaningless, because it always said "yes, that peer is me". This was subsequently fixed (Bug #61787), and now we even have working EXTERNAL authentication on IPv4 sockets by inspecting the Windows equivalent of netstat (same bug) although there is some doubt about whether it is free of race conditions (Bug #83499).
(In reply to Simon McVittie from comment #4) > This entire module is rather suspect ... to which I had intended to add: As a result I'd be tempted to say that in the long term, the nonce-tcp: transport went away completely, and we should use tcp: with EXTERNAL (if the concerns I raised on Bug #83499 turn out to be unfounded) or DBUS_COOKIE_SHA1 (otherwise).
Comment on attachment 135266 [details] [review] do_check_nonce: Don't free uninitialized memory on OOM Review of attachment 135266 [details] [review]: ----------------------------------------------------------------- Yup, r+.
Comment on attachment 135267 [details] [review] do_noncefile_create: Avoid freeing uninitialized memory on error Review of attachment 135267 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 135268 [details] [review] _dbus_accept_with_noncefile: Don't leak nonce Review of attachment 135268 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 135269 [details] [review] DBusNonceFile: Don't rely on caller preallocating the object Review of attachment 135269 [details] [review]: ----------------------------------------------------------------- r+ with two nitpicks (fix before pushing, or disagree and don’t fix; doesn’t matter to me). ::: dbus/dbus-nonce.c @@ +404,5 @@ > > /** > * deletes the noncefile and frees the DBusNonceFile object. > * > + * @param noncefile_location the nonce file to delete. Contents will be freed. Nitpick: ‘Contents will be freed and cleared to #NULL’? @@ +452,5 @@ > * > + * If noncefile_location points to #NULL, nothing is freed or deleted, > + * similar to dbus_error_free(). > + * > + * @param noncefile_location the nonce file to delete. Contents will be freed. Nitpick: ‘Contents will be freed and cleared to #NULL’?
(In reply to Simon McVittie from comment #4) > Created attachment 135269 [details] [review] > DBusNonceFile: Don't rely on caller preallocating the object This one needs minor changes to fix the build for Windows.
Created attachment 135283 [details] [review] DBusNonceFile: Don't rely on caller preallocating the object --- Fix incorrect "return;" in the Windows code path which should have been "return TRUE;". Adjust comments as Philip suggested. Copy "If noncefile_location points to #NULL, nothing is freed or deleted, similar to dbus_error_free()" from one implementation of _delete() to the other.
Comment on attachment 135283 [details] [review] DBusNonceFile: Don't rely on caller preallocating the object Review of attachment 135283 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 135266 [details] [review] do_check_nonce: Don't free uninitialized memory on OOM Fixed in git for 1.12.2, 1.13.0
Comment on attachment 135267 [details] [review] do_noncefile_create: Avoid freeing uninitialized memory on error Fixed in git for 1.12.2, 1.13.0
Comment on attachment 135268 [details] [review] _dbus_accept_with_noncefile: Don't leak nonce Fixed in git for 1.12.2, 1.13.0
Comment on attachment 135283 [details] [review] DBusNonceFile: Don't rely on caller preallocating the object Fixed in git for 1.13.0 (this is really just cleanup so not 1.12.x)
Description:- Steps to reproduce defect: 1.login to gmail application -username : test@gmail.com -password : testing@123 2.click on windows
(In reply to SASI from comment #17) > Description:- > Steps to reproduce defect: > 1.login to gmail application > -username : test@gmail.com > -password : testing@123 > 2.click on windows I think you’ve commented on the wrong bug.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.