search

sftcd / openssl

TLS/SSL and crypto library
https://www.openssl.org
Apache License 2.0
46 stars 20 forks source link

code auditing #16

Open eighthave opened 3 years ago

eighthave commented 3 years ago

I'm working through auditing the ECH code now, and will post findings as put them together. One quick thing is that there are many lines with trailing spaces, which might annoy upstream. Those can be removed with:

find * -type f -print0 | xargs -0 sed -i 's, *$,,'

Here's the first notable thing found with cppcheck:

eighthave commented 3 years ago
sftcd commented 3 years ago

I'm working through auditing the ECH code now, and will post findings as put them together. One quick thing is that there are many lines with trailing spaces, which might annoy upstream. Those can be removed with:

find * -type f -print0 | xargs -0 sed -i 's, *$,,'

Just pushed changes for this. I didn't use the above as there are files that a) I didn't modify for ECH so don't want to for this, and b) that legitimately have spaces at line ends (e.g. some test data).

sftcd commented 3 years ago

Here's the first notable thing found with cppcheck:

  • [x] [apps/s_server.c:595]: (style) Obsolete function 'asctime' called. It is recommended to use 'strftime' instead.

    • there are no existing refs to asctime or strftime
    • there is get_current_time(&timenow)

Not sure I agree asctime() is obsolete and get_current_time wouldn't be right anyway. I did however change to use of the thread-safe gmtime_r and asctime_r as a result of this, so it was useful anyway

Ok - it looks like POSIX does deprecate asctime so I moved to strftime.

sftcd commented 3 years ago

In the LARGE list above, a number of the nits aren't in ECH code, (e.g. number 2 and 4) so I'm just checking those off. For those that are obvious enough ECH things, if I check-em without comment then I've just done the obvious fix following the recommendation.