Re: [sqlite] Coding standard

[Richard Hipp]

On 12/12/19, Richard Hipp  wrote:
> On 12/12/19, Arthur Blondel  wrote:

> Authur sent me a spreadsheet with 432 warnings (not 32000 as
> originally mentioned).

Apparently the first list the OP sent was filtered to show only
"security" warnings.  Arthur sent me the complete list of 31859
warnings this morning, which I have uploaded to
https://sqlite.org/tmp/complete_codesonar_warnings.csv for your
perusal.

I did some spot-checking.  The additional warnings are no better than
the first set, and in some cases quite a bit worse.  I'll have more to
say about CodeSonar later - I have some travel over the next few days
so it might be late in the week before I can get back to this...

All of the warnings are against this check-in:
https://sqlite.org/src/info/18db032d058f1436

-- 
D. Richard Hipp
d...@sqlite.org
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Coding standard

[Jens Alfke]

—Jens 

> On Dec 12, 2019, at 11:23 AM, Warren Young  wrote:
> 
> I wouldn’t dismiss this warning

I wouldn’t dismiss a warning about the full scenario. (In fact I wasn’t aware 
that assignment to a field might overwrite pad bytes; that’s good to know.)

But warning about every call to memset is counterproductive, because it’s much 
too noisy. Memset is used often in situations other than zeroing padded 
structures. There are common ways to zero structs that don’t involve memset— 
like initializing it with “= {}” or using calloc to allocate one on the heap. 
And probably 99% of the time a struct is zeroed, it’s not going to be passed 
across a trust boundary.

It’s kind of like your mom warning you every time you get on your bike, because 
one time a kid rode their bike up to the old quarry and went swimming and 
drowned.

—Jens
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Coding standard

[Scott Robison]

On Thu, Dec 12, 2019, 11:04 PM Valentin Davydov 
wrote:

> On Thu, Dec 12, 2019 at 11:19:44AM -0500, Richard Hipp wrote:
> >
> > #define sqlite3Strlen30NN(C) (strlen(C)&0x3fff)
> >
> > The tool does not provide any details beyond "Use of strlen".
>
> So why not just #define sqlite3Strlen30NN(C) (strnlen(C,0x3fff))
>

strnlen is not in the standard library. It is available on many platforms
but is not standard.
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Coding standard

[Valentin Davydov]

On Thu, Dec 12, 2019 at 11:19:44AM -0500, Richard Hipp wrote:
> 
> #define sqlite3Strlen30NN(C) (strlen(C)&0x3fff)
> 
> The tool does not provide any details beyond "Use of strlen".

So why not just #define sqlite3Strlen30NN(C) (strnlen(C,0x3fff)) ?
From the point of view of program logic it looks similar (at least for 
me), but shifts security burden from you to authors of libc. And of course
this should calm static analyzers anxious about strlen(), sprintf() etc.

Valentin Davydov.

___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Coding standard

[Richard Hipp]

On 12/12/19, Warren Young  wrote:
>
>> AND only if the struct is then copied to a separate trust domain.
>
> You mean like in copying from kernel space to user space?  Or old-style RPC?
>  Or mmap() based IPC APIs?  Or…?
>
> I wouldn’t dismiss this warning.

Copying from kernel-space to user-space is one example.  Another would
be copying an internal SQLite struct out into a file, such as a
database file, accessible to other processes.  SQLite never does
either of these things.  Database files are written byte-by-byte, and
never by copying structures.  We know this is the case because there
is no other way to make the database endian-agnostic.  So the whole
issue is moot and you can ignore all of those warnings.


-- 
D. Richard Hipp
d...@sqlite.org
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Coding standard

[Warren Young]

On Dec 12, 2019, at 12:12 PM, Jens Alfke  wrote:
> 
> On Dec 12, 2019, at 10:36 AM, Simon Slavin  wrote:
>> 
>> For instance CodeSonar reports every use of memset() because you /can/ leak 
>> uninitialised bits of memory using memset() 
> 
> ...by writing to a field of a struct AFTER zeroing the struct with memset

A very common practice, particularly in C, which lacks constructors.

> AND only if the struct field has padding

As a great many structs do, since it’s become passé to optimize struct layout 
for optimal byte packing.

> AND only if the compiler optimizes the write in a particular way

There must be compilers that do this, else no one would be worried about it.  
Bet on it: this rule was written after damage had occurred somewhere, probably 
multiple places, not in anticipation of future damage.

> AND only if the struct is then copied to a separate trust domain.

You mean like in copying from kernel space to user space?  Or old-style RPC?  
Or mmap() based IPC APIs?  Or…?

I wouldn’t dismiss this warning.
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Coding standard

[Jens Alfke]

> On Dec 12, 2019, at 10:36 AM, Simon Slavin  wrote:
> 
> For instance CodeSonar reports every use of memset() because you /can/ leak 
> uninitialised bits of memory using memset() (CERT C Section 3.6 DCL39-C).
> But it has no way to check whether what you're doing with memset() does 
> initialise all bits.

That seems like a silly warning to produce. Reading the CERT section in 
question*, the real  issue is caused not by memset but by writing to a field of 
a struct AFTER zeroing the struct with memset, AND only if the struct field has 
padding, AND only if the compiler optimizes the write in a particular way, AND 
only if the struct is then copied to a separate trust domain.

I don't know anything about CodeSonar, but this particular warning sounds like 
it was added only because it's easy to implement with 'grep'; whereas a useful 
warning that detects the actual situation described by CERT requires something 
on the order of valgrind or the Clang Address Sanitizer, to detect which bytes 
within the struct actually are garbage.

—Jens

* 
https://resources.sei.cmu.edu/downloads/secure-coding/assets/sei-cert-c-coding-standard-2016-v01.pdf

___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Coding standard

[Simon Slavin]

Thanks for the details.  Now we know what you're doing.

CodeSonar checks code for many possible faults.  This includes many things 
which are not related to the CERT C Coding Standard guidelines.  So not 
everything on the report is a violation of the standard.  You're really asking 
something about CodeSonar, not CERT.

For instance CodeSonar reports every use of memset() because you /can/ leak 
uninitialised bits of memory using memset() (CERT C Section 3.6 DCL39-C).
But it has no way to check whether what you're doing with memset() does 
initialise all bits.  And the solution CERT suggests – a substitute for 
memset() – is specific to the architecture of one class of CPUs.  Not useful 
for SQLite which has to run on pretty-much everything.

CodeSonar also assumes an insecure memory model, one where every piece of 
memory is leaked.  For instance, it assumes you're writing kernal code running 
in memory which might be leaked to a user.

I'm a poor C programmer.  (I use C only on tiny embedded devices and have never 
had a job which required me to write C code.).  I might try a tool like 
CodeSonar to catch my poor assumptions and poor techniques.  But it's not up to 
the professionalism of the SQLite devs.

If your bosses require CERT compliance, that's fine.  They're welcome to call 
in a human to check SQLite for violations, and I'm sure the SQLite devs would 
love to know anything found.  But we don't have software that good yet.
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Coding standard

[Richard Hipp]

On 12/12/19, Arthur Blondel  wrote:
> I'm using amalgamation version 3.30.1. and check with CodeSonar

I've never heard of CodeSonar before. It appears to be some kind of
static analysis tool that reads the source code and tries to infer
potential problems.  These tools are usually pretty weak, giving lots
of false-positives.  CodeSonar seems to fall into the general pattern.

Authur sent me a spreadsheet with 432 warnings (not 32000 as
originally mentioned).  Let's take the last one as an example.  The
information I have is:

"Use of strlen"  sqlite3.c:117554

The line of code it is concerned about is here:
https://www.sqlite.org/src/artifact/40557ebd69f4115e?ln=152

Note:  sqlite3Strlen30NN() used to be a real function, but we later
changed it into a macro for performance reasons.  The definition is:

#define sqlite3Strlen30NN(C) (strlen(C)&0x3fff)

The tool does not provide any details beyond "Use of strlen".  Perhaps
it is concerned that the input pointer zColAff is NULL.  The assert()
on the previous line show that concern to be misplaced.  But even
without the assert(), if you trace the logic in this one function, you
clearly see that there is no possible way for zColAff to be NULL.  Of
course, reasoning that through is probably beyond the limited
capabilities of CodeSonar, and so it raised the warning.

Perhaps the concern is that the zColAff string is not zero-terminated.
That case is more difficult to reason about.  If the code between
lines 135 to 149 runs, then zColAff will clearly be zero-terminated,
but what if that logic is skipped?  Who is to say that the string
initially in pTab->zColAff is zero-terminated?  Answering that
question requires a global analysis.  If you examine the rest of the
source code, you find that the *only* place where pTab->zColAff is
ever set is in this one function, and hence pTab->zColAff must always
be zero-terminated.

So any way you look at it, this is a false-positive warning.

And false-positive warnings like this are typical of static analysis
tools.  Indeed, static analysis tools have never proven helpful in
finding problems in SQLite.  SQLite uses more advanced techniques such
as 100% MC/DC testing, run-time analysis, and the latest
profile-guided fuzzing methods.  This is not to say that SQLite is
100% bug-free.  (It isn't.)  But I do claim that the kinds of bugs
that remain in SQLite are way, way beyond the ability of CodeSonar to
detect.

I have not looked at any of the other 441 warnings that Arthur sent,
as my prior experience with these things tells me that they are likely
to all be false-positives.  Time spent analyzing the other 441
warnings is time that I am not using more advanced methods that have a
much higher probability of finding real errors.  So I'm going to
ignore the rest of the CodeSonar warnings and focus on more productive
pursuits.

If anybody else wants to have a look, the list of warnings can be seen
at https://sqlite.org/tmp/codesonar_warnings.csv
-- 
D. Richard Hipp
d...@sqlite.org
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Coding standard

[Richard Hipp]

Also, please send the specific version number (the HASH) of the SQLite
you are testing, so that the line numbers in the report will align
with the code that I will be looking at.

On 12/12/19, Arthur Blondel  wrote:
> -- Forwarded message -
> From: Arthur Blondel 
> Date: Thu, Dec 12, 2019 at 12:19 PM
> Subject: Re: [sqlite] Coding standard
> To: SQLite mailing list 
>
>
> I'm using amalgamation version 3.30.1. and check with CodeSonar
> Attached the Security issues.
>
>
> On Wed, Dec 11, 2019 at 4:57 PM Richard Hipp  wrote:
>
>> On 12/11/19, Arthur Blondel  wrote:
>> > Hello,
>> > Running the CERT coding standard
>> > <https://en.wikipedia.org/wiki/CERT_C_Coding_Standard> on the sqlite
>> code I
>> > get up to 32000 warnings, most of them are security issues.
>>
>> These are all likely to be false-positives.  But if you will send me
>> the list, I will have a look.
>>
>>
>> > Does anyone had this problem? The CERT standard is a must for my
>> > project
>> > and I wonder if there is a solution.
>> > Thanks
>> > ___
>> > sqlite-users mailing list
>> > sqlite-users@mailinglists.sqlite.org
>> > http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
>> >
>>
>>
>> --
>> D. Richard Hipp
>> d...@sqlite.org
>>
>


-- 
D. Richard Hipp
d...@sqlite.org
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Coding standard

[Richard Hipp]

On 12/12/19, Arthur Blondel  wrote:
> I'm using amalgamation version 3.30.1. and check with CodeSonar
> Attached the Security issues.

This mailing list strips attachments.  Please send via private email.

-- 
D. Richard Hipp
d...@sqlite.org
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Coding standard

[Arthur Blondel]

I'm using amalgamation version 3.30.1. and check with CodeSonar
Attached the Security issues.


On Wed, Dec 11, 2019 at 4:57 PM Richard Hipp  wrote:

> On 12/11/19, Arthur Blondel  wrote:
> > Hello,
> > Running the CERT coding standard
> >  on the sqlite
> code I
> > get up to 32000 warnings, most of them are security issues.
>
> These are all likely to be false-positives.  But if you will send me
> the list, I will have a look.
>
>
> > Does anyone had this problem? The CERT standard is a must for my project
> > and I wonder if there is a solution.
> > Thanks
> > ___
> > sqlite-users mailing list
> > sqlite-users@mailinglists.sqlite.org
> > http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
> >
>
>
> --
> D. Richard Hipp
> d...@sqlite.org
>
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Coding standard

[Simon Slavin]

On 11 Dec 2019, at 2:53pm, Arthur Blondel  wrote:

> Running the CERT coding standard
>  on the sqlite code I
> get up to 32000 warnings, most of them are security issues.

The standard itself is good.  But software which looks for violations of the 
standard might not be.  Frankly, there's as much chance of there being bugs in 
their analysis software as there is of bugs in SQLite.  Especially since it 
flags 32000 warnings, which looks like someone's idea of a maximum.

Can you tell us which package you ran and where we can download it from ?  
Also, did you run the software on the full source code tree or the amalgamation 
?
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Coding standard

[Richard Hipp]

On 12/11/19, Arthur Blondel  wrote:
> Hello,
> Running the CERT coding standard
>  on the sqlite code I
> get up to 32000 warnings, most of them are security issues.

These are all likely to be false-positives.  But if you will send me
the list, I will have a look.


> Does anyone had this problem? The CERT standard is a must for my project
> and I wonder if there is a solution.
> Thanks
> ___
> sqlite-users mailing list
> sqlite-users@mailinglists.sqlite.org
> http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
>


-- 
D. Richard Hipp
d...@sqlite.org
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

[sqlite] Coding standard

[Arthur Blondel]

Hello,
Running the CERT coding standard
 on the sqlite code I
get up to 32000 warnings, most of them are security issues.
Does anyone had this problem? The CERT standard is a must for my project
and I wonder if there is a solution.
Thanks
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users