Opened at 2014-06-24T09:38:37Z
Closed at 2014-07-09T11:57:34Z
#1159 closed defect (fixed)
The main() function in unix.c is full of lies
Reported by: | dx | Owned by: | pesco |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | OTR | Version: | 3.2.1 |
Keywords: | Cc: | ||
IRC client+version: | Client-independent | Operating System: | Other |
OS version/distro: |
Description
Since OFTC is being awful right now, this is quite long, and I need to ping pesco, opening a ticket here.
I'm trying to fix #785. It seemed like a simple fix initially, just moving code around, but the main() function is a mess.
In particular, there's a comment written by pesco in 2008 that says that SSL must be initialized before libotr
/* Ugly Note: libotr and gnutls both use libgcrypt. libgcrypt has a process-global config state whose initialization happpens twice if libotr and gnutls are used together. libotr installs custom memory management functions for libgcrypt while our gnutls module uses the defaults. Therefore we initialize OTR after SSL. *sigh* */
Followed by ssl_init() and otr_init().
That was 2008. Since then, a few things changed.
r667.1.9, September 30 2010, support OTR as a plugin, this comment was added after the #ifdef OTR_BI:
/* And in case OTR is loaded as a plugin, it'll also get loaded after this point. */
r702, October 16 2010, call nogaim_init() only once except when using purple, this was added right before the libgcrypt comment.
/* libpurple doesn't like fork()s after initializing itself, so if we use it, do this init a little later (in case we're running in ForkDaemon mode). */ #ifndef WITH_PURPLE nogaim_init(); #endif
And it turns out, nogaim_init() does plugin initialization. Which includes libotr.
So to summarize in a cute table:
Purple | OTR | Protocols | SSL | OTR | Comment |
No | Builtin | 1st | 2nd | 3rd | Right |
No | Plugin | 1st | 3rd | 2nd | Wrong |
Yes | Builtin | 3rd | 1st | 2nd | Right |
Yes | Plugin | 2nd | 1st | 3rd | Right |
First two columns for the compile flags, next three for the initialization order of each of those. "Protocols" being whatever nogaim_init() does - and it happens to have plugin initialization at the end.
So the most common setup (having otr as a plugin and no libpurple) shows the first two comments above are probably a lie.
So, initializing libotr before gnutls certainly doesn't result in crashes.
- Why was that first comment added, then?
- Is the only side effect of this that free()d memory doesn't get completely wiped as otr wants to, but it uses the gnutls methods instead?
- Or was this something that was broken back in 2008 but two years of gnutls updates later it turned out to be ok? mem.c in libotr doesn't seem to have changed at all between 3.2.1 and 4.0.0.
- And most importantly: can I ignore this bullshit and move the ssl initialization after the fork?
Attachments (0)
Change History (4)
comment:1 Changed at 2014-07-03T00:03:46Z by
comment:2 Changed at 2014-07-04T10:21:31Z by
I brought up the issue of otr's mem.c shittiness in #otr, they basically told me to write to the mailing list so ian can know about the issue. Currently writing that email, stay tuned!
But while doing research to avoid saying dumb stuff, I found that gnutls doesn't set libgcrypt allocation handlers since 2008:
http://git.savannah.gnu.org/cgit/gnutls.git/commit/?h=c75014b0776eff5c907d028d2fcd945ff73fc63e
The thread that pointed out this issue is very similar to the libotr issue - gnutls used to override those functions for no good reason with malloc wrappers that are not secure at all:
http://lists.gnupg.org/pipermail/gcrypt-devel/2008-December/001415.html
To answer my own questions:
- Even if there were two different custom allocators, it wouldn't segfault. The first one wins, the other gets ignored (it might throw a warning).
- Since gnutls doesn't use libgcrypt secure memory at all nowadays, there's no conflict at all
- Yes, ignoring the side effects of having a custom allocator at all. Of course, if someone else were to set custom allocators before libotr, that would probably be more secure.
- Bingo. The timestamps fit perfectly with this explanation. Thanks past-dx for including them, btw.
- SSL initialization can be moved after the fork, for sure. I can't ignore the issue, though.
Time to keep writing that email.
comment:3 Changed at 2014-07-07T16:07:41Z by
Just to confirm, it does indeed look like that Dec. 2008 change to gnutls fixes exactly the issue that was the reason for my Feb. 2008 comment.
Specifically, one of libotr's gcry_free()
s would segfault because it had been reset to the standard free()
but the pointer would point into the allocated area (after the added length field).
So it appears you can now put the libotr/ssl initialization in any order and remove that comment.
Good detective work, dx!
comment:4 Changed at 2014-07-09T11:57:34Z by
Resolution: | → fixed |
---|---|
Status: | new → closed |
:D
Seems like one of my conclusions was incorrect - i'm surprised there was a segfault, but i guess nobody would have noticed otherwise.
Either way - added a patch that fixes the original bug to #785, and removes the misleading comments.
The patch isn't merged yet, but we can consider this issue fixed. Closing!
Vaguely relevant bug in the otr bugtracker:
https://bugs.otr.im/issues/23
If i'm reading this right, this secure memory handling is not only pointless but also potentially insecure. Yay. When the hell will they release bugfixes for 4.0.0?