ELL static analysis

ELL static analysis

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 2836 bytes --]


I ran ELL through a static analysis tool and came up with a lot of hits. 
Only a handful were bogus, and there are also some that definitely require 
fixes (I'll send patches).

There are a few broad classes of issues that I wanted to get some feedback 
on before making changes.


1. Ignored returned error values from l_io_get_fd()

l_io_get_fd() returns -1 when it's passed a NULL l_io pointer. In many 
places, that -1 gets passed to a system call as a file descriptor. Those 
system calls typically return an error when they are given a bad fd, and 
there is typically proper error handling for the system call.

Example: 
https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/netlink.c?id=a5afb52d308d9154efa26cce962572f7724e3684#n240

Many of these cases involve passing in a pointer that's only used by ELL 
internals and is never set to NULL anyway. What do you think about leaving 
these alone as long as there's a defined error path (even if that error 
path involves a system call that could have been skipped)?


2. Ignored returned error values from l_hashmap_insert()

l_hashmap_insert() returns false when passed a NULL l_hashmap pointer. 
This isn't always checked, especially when the l_hashmap is something used 
by the ELL internals and is never set to NULL.

Example: 
https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/dbus.c?id=a5afb52d308d9154efa26cce962572f7724e3684#n821

Seems like these are low risk if it can be confirmed that the pointers 
aren't set to NULL during their owner's lifetime.


3. strcpy() instances that could easily be strncpy()

A number of calls to strcpy() were flagged that are copying to 
destinations of known length.

Example:
https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/gvariant-util.c?id=a5afb52d308d9154efa26cce962572f7724e3684#n836

These should get fixed, especially where the source string comes from 
non-ELL code. I can't think of a reason to use strcpy() over strncpy() 
unless the source string is hard-coded, and even then strncpy() isn't a 
burden.


4. Ignored errors in test/example code

These are low risk because they aren't in the library itself. However, 
tests and examples are used as templates for new programs. Does it make 
sense to make the examples (and maybe the tests) squeaky-clean in terms of 
error handling?

Example:

https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/examples/https-server-test.c?id=a5afb52d308d9154efa26cce962572f7724e3684#n128



To take a step back, it's one thing to fix these bugs now, but another to 
have something in place to catch future problems. I think it would be good 
to set up ELL for scanning on an opensource-friendly static analysis site 
- I'll volunteer to babysit that process too.


--
Mat Martineau
Intel OTC

Re: ELL static analysis

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 4656 bytes --]

Hi Mat,

On 09/20/2017 06:40 PM, Mat Martineau wrote:
> 
> I ran ELL through a static analysis tool and came up with a lot of hits. 
> Only a handful were bogus, and there are also some that definitely 
> require fixes (I'll send patches).
> 
> There are a few broad classes of issues that I wanted to get some 
> feedback on before making changes.
> 
> 
> 1. Ignored returned error values from l_io_get_fd()
> 
> l_io_get_fd() returns -1 when it's passed a NULL l_io pointer. In many 
> places, that -1 gets passed to a system call as a file descriptor. Those 
> system calls typically return an error when they are given a bad fd, and 
> there is typically proper error handling for the system call.
> 
> Example: 
> https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/netlink.c?id=a5afb52d308d9154efa26cce962572f7724e3684#n240 
> 
> 
> Many of these cases involve passing in a pointer that's only used by ELL 
> internals and is never set to NULL anyway. What do you think about 
> leaving these alone as long as there's a defined error path (even if 
> that error path involves a system call that could have been skipped)?
> 

I bet almost all of these are inside read/write handlers.  read/write 
handlers will not be called on a NULL io object or if the underlying fd 
becomes invalid.  So you're guaranteed that the io object is valid. 
These can be left alone.

> 
> 2. Ignored returned error values from l_hashmap_insert()
> 
> l_hashmap_insert() returns false when passed a NULL l_hashmap pointer. 
> This isn't always checked, especially when the l_hashmap is something 
> used by the ELL internals and is never set to NULL.
> 
> Example: 
> https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/dbus.c?id=a5afb52d308d9154efa26cce962572f7724e3684#n821 
> 
> 
> Seems like these are low risk if it can be confirmed that the pointers 
> aren't set to NULL during their owner's lifetime.
> 

We can look at these, but generally the hashmaps we use are created at 
object allocation or first insertion and destroyed when the object is 
destroyed.  So likely most of these can be ignored.

> 
> 3. strcpy() instances that could easily be strncpy()
> 
> A number of calls to strcpy() were flagged that are copying to 
> destinations of known length.
> 
> Example:
> https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/gvariant-util.c?id=a5afb52d308d9154efa26cce962572f7724e3684#n836 
> 
> 
> These should get fixed, especially where the source string comes from 

So the particular case you bring up is actually a good example of why we 
_don't_ want to convert every single strcpy into a strncpy.

1. The signature is validated by the logic above to be no more than 255 
characters (e.g. _gvariant_valid_signature).
2. The builder object is meant to be used by the programmer to build 
D-Bus messages.  It isn't meant to receive arbitrary input from the 
network, etc. So in this case, even if somehow a buffer overflow occurs, 
I want valgrind to warn the programmer ASAP and tell them exactly where 
they screwed up instead of having a silent and hard-to-find failure.

Our principle of crash-early applies...

> non-ELL code. I can't think of a reason to use strcpy() over strncpy() 
> unless the source string is hard-coded, and even then strncpy() isn't a 
> burden.
> 

strncpy is a pain in the ass due to null termination semantics and 
strcpy is such an obvious attack vector that any code introduced with it 
gets scrutinized anyway.

I briefly grepped and looked at our use of strcpy and everything I found 
was completely and obviously okay.  Lets have a deeper look, but I'm 
totally not agreeing with your assertion above ;)

> 
> 4. Ignored errors in test/example code
> 
> These are low risk because they aren't in the library itself. However, 
> tests and examples are used as templates for new programs. Does it make 
> sense to make the examples (and maybe the tests) squeaky-clean in terms 
> of error handling?
> 
> Example:
> 
> https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/examples/https-server-test.c?id=a5afb52d308d9154efa26cce962572f7724e3684#n128 
> 
> 

Yeah I'd vote to have that one fixed since the rest of the example 
provides proper error handling.

> 
> 
> To take a step back, it's one thing to fix these bugs now, but another 
> to have something in place to catch future problems. I think it would be 
> good to set up ELL for scanning on an opensource-friendly static 
> analysis site - I'll volunteer to babysit that process too.
> 

That would be nice!

Regards,
-Denis

Re: ELL static analysis

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 5648 bytes --]


On Wed, 20 Sep 2017, Denis Kenzior wrote:

> Hi Mat,
>
> On 09/20/2017 06:40 PM, Mat Martineau wrote:
>> 
>> I ran ELL through a static analysis tool and came up with a lot of hits. 
>> Only a handful were bogus, and there are also some that definitely require 
>> fixes (I'll send patches).
>> 
>> There are a few broad classes of issues that I wanted to get some feedback 
>> on before making changes.
>> 
>> 
>> 1. Ignored returned error values from l_io_get_fd()
>> 
>> l_io_get_fd() returns -1 when it's passed a NULL l_io pointer. In many 
>> places, that -1 gets passed to a system call as a file descriptor. Those 
>> system calls typically return an error when they are given a bad fd, and 
>> there is typically proper error handling for the system call.
>> 
>> Example: 
>> https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/netlink.c?id=a5afb52d308d9154efa26cce962572f7724e3684#n240 
>> 
>> Many of these cases involve passing in a pointer that's only used by ELL 
>> internals and is never set to NULL anyway. What do you think about leaving 
>> these alone as long as there's a defined error path (even if that error 
>> path involves a system call that could have been skipped)?
>> 
>
> I bet almost all of these are inside read/write handlers.  read/write 
> handlers will not be called on a NULL io object or if the underlying fd 
> becomes invalid.  So you're guaranteed that the io object is valid. These can 
> be left alone.

I checked a few more examples - they are mostly read/write handlers.

>
>> 
>> 2. Ignored returned error values from l_hashmap_insert()
>> 
>> l_hashmap_insert() returns false when passed a NULL l_hashmap pointer. This 
>> isn't always checked, especially when the l_hashmap is something used by 
>> the ELL internals and is never set to NULL.
>> 
>> Example: 
>> https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/dbus.c?id=a5afb52d308d9154efa26cce962572f7724e3684#n821 
>> 
>> Seems like these are low risk if it can be confirmed that the pointers 
>> aren't set to NULL during their owner's lifetime.
>> 
>
> We can look at these, but generally the hashmaps we use are created at object 
> allocation or first insertion and destroyed when the object is destroyed.  So 
> likely most of these can be ignored.

Ok.

>
>> 
>> 3. strcpy() instances that could easily be strncpy()
>> 
>> A number of calls to strcpy() were flagged that are copying to destinations 
>> of known length.
>> 
>> Example:
>> https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/gvariant-util.c?id=a5afb52d308d9154efa26cce962572f7724e3684#n836 
>> 
>> These should get fixed, especially where the source string comes from 
>
> So the particular case you bring up is actually a good example of why we 
> _don't_ want to convert every single strcpy into a strncpy.
>
> 1. The signature is validated by the logic above to be no more than 255 
> characters (e.g. _gvariant_valid_signature).
> 2. The builder object is meant to be used by the programmer to build D-Bus 
> messages.  It isn't meant to receive arbitrary input from the network, etc. 
> So in this case, even if somehow a buffer overflow occurs, I want valgrind to 
> warn the programmer ASAP and tell them exactly where they screwed up instead 
> of having a silent and hard-to-find failure.
>
> Our principle of crash-early applies...

Crash early, but only when running valgrind/asan/etc? When it's trivial 
to check for forseeable misuse, especially with regard to string overruns, 
we can deterministically crash (or otherwise fail) early.

>
>> non-ELL code. I can't think of a reason to use strcpy() over strncpy() 
>> unless the source string is hard-coded, and even then strncpy() isn't a 
>> burden.
>>

Ok, one reason: strncpy() has the additional cost of zeroing the unused 
space at the end of the dest buffer, so if the src length is otherwise 
checked strcpy() doesn't hurt.

>
> strncpy is a pain in the ass due to null termination semantics and strcpy is 
> such an obvious attack vector that any code introduced with it gets 
> scrutinized anyway.

Static analysis helps catch strncpy misuse - it caught some off-by-one 
strncpy parameters that we need to fix.

>
> I briefly grepped and looked at our use of strcpy and everything I found was 
> completely and obviously okay.  Lets have a deeper look, but I'm totally not 
> agreeing with your assertion above ;)

Agree with the deeper look. I've also revised my assertation :)

>> 
>> 4. Ignored errors in test/example code
>> 
>> These are low risk because they aren't in the library itself. However, 
>> tests and examples are used as templates for new programs. Does it make 
>> sense to make the examples (and maybe the tests) squeaky-clean in terms of 
>> error handling?
>> 
>> Example:
>> 
>> https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/examples/https-server-test.c?id=a5afb52d308d9154efa26cce962572f7724e3684#n128 
>> 
>
> Yeah I'd vote to have that one fixed since the rest of the example provides 
> proper error handling.
>
>> 
>> 
>> To take a step back, it's one thing to fix these bugs now, but another to 
>> have something in place to catch future problems. I think it would be good 
>> to set up ELL for scanning on an opensource-friendly static analysis site - 
>> I'll volunteer to babysit that process too.
>> 
>
> That would be nice!

I got this mostly set up today, and am waiting for approval. I'll post 
information to the list once the project is approved and accessible.

--
Mat Martineau
Intel OTC