On Friday, May 10, 2019 12:13:32 AM CEST Bruno Haible wrote: > Hi Paul, > > > Sorry, I'm still not following. Unless the tainted data is used to > > calculate an array index, there's no problem with Heartbleed and the > > Coverity heuristic should not diagnose a problem. > > Yes, IF they were only using an algorithm and no heuristic, > base64_encode would not be flagged as a dangerous consumer of > untrusted input. > > But their article > https://www.synopsys.com/blogs/software-security/detecting-heartbleed-with-s > tatic-analysis/ says: > > "Program analysis is hard and approximations and trade-offs are > absolutely mandatory. We’ve found that the best results come from > a combination of advanced algorithms and knowledge of idioms that > occur in real-world code." > > So they are combining data flow analysis - in order to determine > that the argument of base64_alloc is untrusted data - with a > heuristic - "if a function contains array accesses with indices that > are computed with ntohs calls, we should flag it as dangerous consumer". > > > the proposed comment would be wrong as it would pacify > > Coverity without fixing the real bug elsewhere. > > The base64_encode function does not make a dangerous array > access (only to the 'b64c' array). And its result is a string, > not an integer, and therefore cannot be used as an array index > either. Therefore adding this comment cannot silence real bugs. > > But maybe it will be sufficient to mask all b64c arguments > with '& 0x3f', like you already suggested in the other mail? > > Bruno > > diff --git a/lib/base64.c b/lib/base64.c > index f3f7298..a00e0f4 100644 > --- a/lib/base64.c > +++ b/lib/base64.c > @@ -70,7 +70,7 @@ base64_encode_fast (const char *restrict in, size_t inlen, > char *restrict out) { > while (inlen) > { > - *out++ = b64c[to_uchar (in[0]) >> 2]; > + *out++ = b64c[(to_uchar (in[0]) >> 2) & 0x3f]; > *out++ = b64c[((to_uchar (in[0]) << 4) + (to_uchar (in[1]) >> 4)) & > 0x3f]; *out++ = b64c[((to_uchar (in[1]) << 2) + (to_uchar (in[2]) >> 6)) & > 0x3f]; *out++ = b64c[to_uchar (in[2]) & 0x3f]; > @@ -103,7 +103,7 @@ base64_encode (const char *restrict in, size_t inlen, > > while (inlen && outlen) > { > - *out++ = b64c[to_uchar (in[0]) >> 2]; > + *out++ = b64c[(to_uchar (in[0]) >> 2) & 0x3f]; > if (!--outlen) > break; > *out++ = b64c[((to_uchar (in[0]) << 4) > > Bruno
Thanks! This also helps to suppress the false positives on cryptsetup with Coverity Static Analysis version 2019.03. Kamil
