[redis-oss] Avoid using unsafe C functions by ranshid · Pull Request #10932 · redis/redis

ranshid · 2022-07-04T14:51:18Z

replace use of:
sprintf --> snprintf
strcpy/strncpy --> redis_strlcpy
strcat/strncat --> redis_strlcat

why are we making this change?
Much of the code uses some unsafe variants or deprecated buffer handling
functions.
While most cases are probably not presenting any issue on the known path
programming errors and unterminated strings might lead to potential
buffer overflows which are not covered by tests.

As part of this PR we change

added implementation for redis_strlcpy and redis_strlcat based on the strl implementation: https://linux.die.net/man/3/strl
change all occurrences of use of sprintf with use of snprintf
change occurrences of use of strcpy/strncpy with redis_strlcpy
change occurrences of use of strcat/strncat with redis_strlcat
change the behavior of ll2string/ull2string/ld2string so that it will always place null termination ('\0') on the output buffer in the first index. this was done in order to make the use of these functions more safe in cases were the user will not check the output returned by them (for example in rdbRemoveTempFile)
we added a compiler directive to issue a deprecation error in case a use of sprintf/strcpy/strcat is found during compilation which will result in error during compile time. However keep in mind that since the deprecation attribute is not supported on all compilers, this is expected to fail during push workflows.

NOTE: while this is only an initial milestone. We might also consider
using the *_s implementation provided by the C11 Extensions (however not
yet widly supported). I would also suggest to start
looking at static code analyzers to track unsafe use cases.
For example LLVM clang checker supports security.insecureAPI.DeprecatedOrUnsafeBufferHandling
which can help locate unsafe function usage.
https://clang.llvm.org/docs/analyzer/checkers.html#security-insecureapi-deprecatedorunsafebufferhandling-c
The main reason not to onboard it at this stage is that the alternative
excepted by clang is to use the C11 extensions which are not always
supported by stdlib.

replace use of: sprintf --> snprintf strcpy/strncpy --> strlcpy strcat/strncat --> strlcat why are we making this change? Much of the code uses some unsafe variants or deprecated buffer handling functions. While most cases are probably not presenting any issue on the known path programming errors and unterminated strings might lead to potential buffer overflows which are not covered by tests.

madolson · 2022-07-04T15:17:07Z

I'll add one other piece of context, which is that AFAIK Redis never uses any of these "unsafe" functions unsafely, however we are constantly contacted by folks from our internal security teams about using the unsafe functions, so I'm fairly confident other folks do as well. I know we've also had past individuals have used static analysis tools to try to find issues in Redis, and I know these are often flagged and end up being false positives.

The main benefit I see is that in the future people will be nudged towards the correct functions.

oranagra

just to note again (what was already note in some way):
many of these changes are to test code that's not really part of the normal redis build.
and the ones that are in real redis code seem bug free to me too.
i do agree we wanna do this step towards eliminating static analysis warnings.

src/anet.c

src/rdb.c

src/util.c

src/redis-cli.c

src/util.c

1. some limits for strlcpy were not modified to copy length 2. revert use of snprintf and use strlcat in rdbRemoveTempFile since it is called on signal handler

ranshid · 2022-07-05T06:39:10Z

just to note again (what was already note in some way): many of these changes are to test code that's not really part of the normal redis build. and the ones that are in real redis code seem bug free to me too. i do agree we wanna do this step towards eliminating static analysis warnings.

@oranagra I agree there is no bug in the current use of these functions. the main Idea with this PR (while it was initiated by a static analyzer warning) is to also make sure no future use will be done using these functions, since there is no visible reason to do so.
On the PR description I placed a note suggesting we also take the extra mile to integrate some static code analyzer as part of our CI. While usually static code analyzers will have many false positive, I believe that correctly fine-tuning them can help reducing different issues during CR and contribute to better code quality. We are currently making an effort to evaluate if static code analyzing can locate errors or offer good change suggestion, but would love to also have your view of such an effort.

oranagra · 2022-07-05T07:25:56Z

we do already have one static analyzer running as part of the CI, maybe we can extend that and add others.
i do agree we wanna silence any warnings / concerns rather than evaluate them and ignore the reports as false positives.

madolson

lgtm

oranagra

i'd like @yossigo to take a quick look (at the copied code, makefile, and pragmas)

src/Makefile

src/rdb.c

src/config.h

src/redis-cli.c

src/zmalloc.c

1. fix some spaces in command arguments 2. place all strl* implementations in separate c file strl.c 3. remove handling of theoretical pid 2 string error

oranagra

other than some minor changes, LGTM.

src/util.c

src/rdb.c

src/dict.c

src/util.c

Co-authored-by: sundb <[email protected]>

oranagra

LGTM.

@ranshid please mention the changes we did to ll2string etc in the PR top comment.
i.e. explain that we added a null term in the error case just in case some caller (like rdbRemoveTempFile) doesn't check the error and assumes the string has some null term.
please go over all the changes and see if there's anything else worth mentioning.

@yossigo i'd like you to take a quick look (mainly at the copied code, makefile, and pragmas)

ranshid · 2022-07-17T10:08:52Z

LGTM.

@ranshid please mention the changes we did to ll2string etc in the PR top comment. i.e. explain that we added a null term in the error case just in case some caller (like rdbRemoveTempFile) doesn't check the error and assumes the string has some null term. please go over all the changes and see if there's anything else worth mentioning.

@oranagra I updated the PR top comment. take a look if you like and tell me if that is O.K now.

yossigo

LGTM.

replace use of: sprintf --> snprintf strcpy/strncpy --> redis_strlcpy strcat/strncat --> redis_strlcat **why are we making this change?** Much of the code uses some unsafe variants or deprecated buffer handling functions. While most cases are probably not presenting any issue on the known path programming errors and unterminated strings might lead to potential buffer overflows which are not covered by tests. **As part of this PR we change** 1. added implementation for redis_strlcpy and redis_strlcat based on the strl implementation: https://linux.die.net/man/3/strl 2. change all occurrences of use of sprintf with use of snprintf 3. change occurrences of use of strcpy/strncpy with redis_strlcpy 4. change occurrences of use of strcat/strncat with redis_strlcat 5. change the behavior of ll2string/ull2string/ld2string so that it will always place null termination ('\0') on the output buffer in the first index. this was done in order to make the use of these functions more safe in cases were the user will not check the output returned by them (for example in rdbRemoveTempFile) 6. we added a compiler directive to issue a deprecation error in case a use of sprintf/strcpy/strcat is found during compilation which will result in error during compile time. However keep in mind that since the deprecation attribute is not supported on all compilers, this is expected to fail during push workflows. **NOTE:** while this is only an initial milestone. We might also consider using the *_s implementation provided by the C11 Extensions (however not yet widly supported). I would also suggest to start looking at static code analyzers to track unsafe use cases. For example LLVM clang checker supports security.insecureAPI.DeprecatedOrUnsafeBufferHandling which can help locate unsafe function usage. https://clang.llvm.org/docs/analyzer/checkers.html#security-insecureapi-deprecatedorunsafebufferhandling-c The main reason not to onboard it at this stage is that the alternative excepted by clang is to use the C11 extensions which are not always supported by stdlib.

oranagra reviewed Jul 4, 2022

View reviewed changes

src/anet.c Outdated Show resolved Hide resolved

src/rdb.c Outdated Show resolved Hide resolved

src/util.c Outdated Show resolved Hide resolved

src/redis-cli.c Outdated Show resolved Hide resolved

src/util.c Outdated Show resolved Hide resolved

ranshid added 2 commits July 4, 2022 22:34

fix some cr comments:

aac0422

1. some limits for strlcpy were not modified to copy length 2. revert use of snprintf and use strlcat in rdbRemoveTempFile since it is called on signal handler

Place strlcpy and strcpy implementation before REDIS_TEST definition

7a4626b

rename strlcpy and strlcat to redis_strlcpy and redis_strlcat

62305c6

madolson requested a review from oranagra July 12, 2022 05:03

madolson approved these changes Jul 12, 2022

View reviewed changes

oranagra reviewed Jul 12, 2022

View reviewed changes

src/Makefile Outdated Show resolved Hide resolved

src/rdb.c Outdated Show resolved Hide resolved

sundb reviewed Jul 13, 2022

View reviewed changes

src/config.h Outdated Show resolved Hide resolved

sundb reviewed Jul 13, 2022

View reviewed changes

src/redis-cli.c Outdated Show resolved Hide resolved

src/redis-cli.c Outdated Show resolved Hide resolved

src/zmalloc.c Outdated Show resolved Hide resolved

ranshid added 3 commits July 13, 2022 09:16

Making some fixes according to PR suggestions:

a3704bd

1. fix some spaces in command arguments 2. place all strl* implementations in separate c file strl.c 3. remove handling of theoretical pid 2 string error

change the GCC_VERSION to __GNUC__

43a1346

add protection on ll2string, ull2string and ld2string

3e7393e

oranagra approved these changes Jul 13, 2022

View reviewed changes

src/util.c Outdated Show resolved Hide resolved

src/util.c Show resolved Hide resolved

src/rdb.c Outdated Show resolved Hide resolved

some pr small fixes

79c1a0e

sundb reviewed Jul 15, 2022

View reviewed changes

src/dict.c Outdated Show resolved Hide resolved

src/util.c Outdated Show resolved Hide resolved

ranshid and others added 2 commits July 16, 2022 10:42

Update src/dict.c

af27ace

Co-authored-by: sundb <[email protected]>

Update src/util.c

cb7f220

Co-authored-by: sundb <[email protected]>

oranagra approved these changes Jul 17, 2022

View reviewed changes

yossigo approved these changes Jul 18, 2022

View reviewed changes

oranagra merged commit eacca72 into redis:unstable Jul 18, 2022

Conversation

ranshid commented Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madolson commented Jul 4, 2022

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ranshid commented Jul 5, 2022

Uh oh!

oranagra commented Jul 5, 2022

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

ranshid commented Jul 17, 2022

Uh oh!

yossigo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ranshid commented Jul 4, 2022 •

edited

Loading