Coding standard

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

Coding standard

Arthur Blondel
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.
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
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Coding standard

Richard Hipp-3
On 12/11/19, Arthur Blondel <[hidden email]> 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
> [hidden email]
> http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
>


--
D. Richard Hipp
[hidden email]
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Coding standard

Simon Slavin-3
In reply to this post by Arthur Blondel
On 11 Dec 2019, at 2:53pm, Arthur Blondel <[hidden email]> wrote:

> 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.

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
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Coding standard

Arthur Blondel
In reply to this post by Richard Hipp-3
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 <[hidden email]> wrote:

> On 12/11/19, Arthur Blondel <[hidden email]> 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
> > [hidden email]
> > http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
> >
>
>
> --
> D. Richard Hipp
> [hidden email]
>
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Coding standard

Richard Hipp-3
On 12/12/19, Arthur Blondel <[hidden email]> 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
[hidden email]
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Coding standard

Richard Hipp-3
In reply to this post by Arthur Blondel
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 <[hidden email]> wrote:

> ---------- Forwarded message ---------
> From: Arthur Blondel <[hidden email]>
> Date: Thu, Dec 12, 2019 at 12:19 PM
> Subject: Re: [sqlite] Coding standard
> To: SQLite mailing list <[hidden email]>
>
>
> 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 <[hidden email]> wrote:
>
>> On 12/11/19, Arthur Blondel <[hidden email]> 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
>> > [hidden email]
>> > http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
>> >
>>
>>
>> --
>> D. Richard Hipp
>> [hidden email]
>>
>


--
D. Richard Hipp
[hidden email]
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Coding standard

Richard Hipp-3
In reply to this post by Arthur Blondel
On 12/12/19, Arthur Blondel <[hidden email]> 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)&0x3fffffff)

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
[hidden email]
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Coding standard

Simon Slavin-3
In reply to this post by Arthur Blondel
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
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Coding standard

Jens Alfke-2


> On Dec 12, 2019, at 10:36 AM, Simon Slavin <[hidden email]> 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
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Coding standard

Warren Young
On Dec 12, 2019, at 12:12 PM, Jens Alfke <[hidden email]> wrote:
>
> On Dec 12, 2019, at 10:36 AM, Simon Slavin <[hidden email]> 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
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Coding standard

Richard Hipp-3
On 12/12/19, Warren Young <[hidden email]> 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
[hidden email]
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Coding standard

Valentin Davydov-2
In reply to this post by Richard Hipp-3
On Thu, Dec 12, 2019 at 11:19:44AM -0500, Richard Hipp wrote:
>
>     #define sqlite3Strlen30NN(C) (strlen(C)&0x3fffffff)
>
> The tool does not provide any details beyond "Use of strlen".

So why not just #define sqlite3Strlen30NN(C) (strnlen(C,0x3fffffff)) ?
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
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Coding standard

Scott Robison-2
On Thu, Dec 12, 2019, 11:04 PM Valentin Davydov <[hidden email]>
wrote:

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

strnlen is not in the standard library. It is available on many platforms
but is not standard.
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Coding standard

Jens Alfke-2
In reply to this post by Warren Young


—Jens

> On Dec 12, 2019, at 11:23 AM, Warren Young <[hidden email]> 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
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Coding standard

Richard Hipp-3
In reply to this post by Richard Hipp-3
On 12/12/19, Richard Hipp <[hidden email]> wrote:
> On 12/12/19, Arthur Blondel <[hidden email]> 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
[hidden email]
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users