[OTR-dev] [PATCH] Don't crash when parsing arbitrary string with otrl_proto_query_bestversion()

[Ian Goldberg]

On Sat, Oct 12, 2013 at 10:30:57PM +0200, Conrad Hoffmann wrote:
> Hi folks,
> 
> first things first: a big thank you to all you who have devote time to
> this awesome library - you rock!
> 
> But now enough sweet-talkin' ;)
> 
> I noticed that otrl_proto_query_bestversion() crashes when used on
> arbitrary text that does not contain an OTR query. I admit that that's
> obviously not the purpose of this function, but since it is
>  a) a somewhat high-levelish, easy-to-use-looking interface
>  b) not exactly thoroughly documented ;) and
>  c) a possible attack vector
> I figured it might be worth the time to avoid at least the segfault.
> 
> Here is the current code (comments by me):
> 
>   otrtag = strstr(otrquerymsg, "?OTR"); /* May return NULL! */
>   otrtag += 4;
> 
>   /* First check is pointless, since otrtag was just incremented by 4 */
>   if (otrtag && *otrtag == '?') {
> 
> So if no tag is found, strstr() returns NULL and then the memory address
> 0x4 is fetched, which just might segfault... a lot.
> 
> Attached patch moves the check right after the call to strstr() and gets
> rid other checks since otrtag can't be NULL after the incrementation
> anymore. Or did I miss anything?
> 
> I just subscribed to the list, so reply-to-list is sufficient.
> 
> Cheers and happy hacking,
> Conrad

Applied to git, thanks!

   - Ian