use valgrind / Re: SIGSEGV in cyrus-imapd 3.0.7 mupdate

[ellie timoney]

Hi Michael,

On Fri, Jul 13, 2018, at 6:40 PM, Michael Menge wrote:
> Hi Ellie
> 
> thanks for your replies,
> 
> Quoting ellie timoney <ellie at fastmail.com>:
> 
> > From what I'm seeing here, it looks like mupdate calls  
> > tls_init_serverengine() for each new STARTTLS session, and then  
> > calls tls_shutdown_serverengine()  when that session ends.
> >
> > The thing is though, the TLS state that these functions manage is  
> > something like a singleton, it should only exist once for each  
> > process.  And in fact, tls_init_serverengine() enforces this (which  
> > is why you don't see the dh_params being allocated for each  
> > connection: it only lets itself run once).  This works fine for all  
> > the single-threaded service programs, but mupdate is a threaded  
> > application and can have multiple connected client sessions in the  
> > same process.  The comments at the top of imap/tls.c are instructive:
> >
> >> * DESCRIPTION
> >> *       This module is the interface between Cyrus Imapd and the  
> >> OpenSSL library.
> >> *       As of now only one filedescriptor can be handled, so only one
> >> *       TLS channel can be open at a time.
> >
> > I can't see anything in the code that makes me suspect this comment  
> > is out of date.  It doesn't look the slightest bit thread-safe.
> >
> > At the moment, it looks to me like the STARTTLS support was added to  
> > mupdate with no consideration of its thread-safety, and I'm left to  
> > assume that people aren't really using STARTTLS for their mupdate  
> > connections, otherwise this probably would've been tripped over long  
> > ago.  (Or, maybe it was, and someone said "don't do that", and then  
> > it all got lost to history?  It would be interesting to hear from  
> > the front lines.)
> 
> I have been running mupdate with STARTTLS for a few years with 2.3.x  
> and 2.4.x.
> It didn't crash as predictable as 3.0, but we had some instances where the
> mupdate master crashed and stopped working and we had to restart the service
> (on an other host). At that time we where unable to debug it, as it would
> run without crash for weeks or months. I suspect now that the  
> non-thread-safe usage
> of openssl could have been the cause for this. 

That's interesting to hear.  Have your servers become busier over that time?  I expect that in a low traffic scenario mupdate might end up being mostly single-threaded in practice, allowing the issue to go unobserved, but as traffic increases, the number of conflicts would increase.

> Using STARTTLS for mupdate was the obvious solution after the default  
> for "allowplaintext"
> (Allow the use of cleartext passwords on the wire) was changed to 0.

You _might_ be able to do something with setting "mupdate_allowplaintext: yes" to allow plaintext connections only to the mupdate service without affecting other services, if your only need for STARTTLS in mupdate was to get around that default.  I can't remember how the service-specific config prefixing mechanism works (and can never seem to find it when I look for it in code/docs), so this might or might not actually work...

> >
> > Anyway, it looks to me like the STARTTLS support in mupdate is just  
> > fundamentally broken at the moment, and my recommendation is to not  
> > use it.  If your IMAP servers need to connect to an mupdate server  
> > that's not within their trusted network, I guess you'd need to set  
> > up a VPN for it or something along those lines (but I'm no network  
> > specialist).
> >
> 
> could you add a warning in the relevant murder/installation guides and  
> manuals?

Yep, that's an excellent idea. :)

> 
> > I don't honestly see this being fixed any time soon -- it would  
> > require either:
> >
> >  * a big rewrite of imap/tls.c to make it thread-safe
> >  * a big rewrite of mupdate to make it single-threaded like the rest of Cyrus
> 
> out of curiosity, why is mupdate multi-threaded in the first place?

I asked around last week but it seems like that answer has been lost to history.  It was added a long time ago, so it might have been a "threads are cheaper than processes" decision.  The initial commit for imap/mupdate.c (2001) describes it as "added mupdate for possible lightweight acap replacement", but I don't know what "acap" was!

> 
> >  * a big rewrite of mupdate to make it do its own TLS handling  
> > (rather than using imap/tls.c)
> >
> 
> This sounds like it is the same work as the first item, but it is
> only used for mupdate.
> 
> > All of these are serious tasks with serious testing requirements,  
> > especially considering the need to interact with OpenSSL correctly.   
> > Even if someone does produce patches for master, they won't make it  
> > back to the 3.0 series.
> >
> 
> so is there a chance for a patch in 3.1?

Bron has his infamous "what it would take for FastMail to use murder" list, but the main development on 3.1 at the moment is JMAP and Calendaring and stuff relating to those, and I guess 3.2 stable will probably land once that work is complete.

There's a roadmap of sorts somewhere on this list, but off the top of my head I can't remember where murder improvements fit in.  Wild guess, maybe it'll be on the radar for development on 3.3.

> 
> > Sorry to be the bearer of annoying news!
> 
> I will see how I can work around this.

It'd be great to hear back as to what sort of workarounds you find, so that we might add them to docs too in the meantime.

> The cyrus project has much improved since fastmail dedicated
> some developers to it, but I think it is time that fastmail switches
> to a murder setup, so that this part of the code gets the same love
> and testing as the other parts of cyrus ;-)

Hehe :D

> Thanks to all helping debugging this problem. I will gladly help
> with testing any fixes for this bug.
> 
> Kind regards,
> 
>     Michael

Cheers,

ellie