Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sun, 23 Dec 2012 17:44:21 +0100
From: magnum <>
Subject: Re: scan-build results, part 1

On 23 Dec, 2012, at 17:34 , Dhiru Kholia <> wrote:

> On Sun, Dec 23, 2012 at 9:46 PM, magnum <> wrote:
>> Another weird complaint is for MSCHAPv2. The pos pointer is set to non-null in line 429. How could it ever be a null dereference in line 433?
> I have received an "official" answer on this one. It is *not* a false
> positive. "ciphertext" can be NULL and we haven't checked for it
> before doing pointer arithmetic on line 429.
> gwynne> From the analyzer's point of view, NULL acts like NaN; i.e.
> "NULL + anything = NULL" in terms of pointer validity.
> Following patch makes this problem go away.
> diff --git a/src/MSCHAPv2_fmt_plug.c b/src/MSCHAPv2_fmt_plug.c
> index d946036..b4c7fcf 100644
> --- a/src/MSCHAPv2_fmt_plug.c
> +++ b/src/MSCHAPv2_fmt_plug.c
> @@ -44,6 +44,7 @@
> #include "sha.h"
> #include <openssl/des.h>
> +#include <assert.h>
> ...

Fair enough. But I think I recall Solar wants to avoid using assert() and in this case, we know (even if scan-build doesn't) that this function will never be called with ciphertext being null. So maybe we should just ignore it.

How come it doesn't complain about a hundred other formats' salt(), binary() and valid()? We never test against ciphertext being null, do we?


Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.