src/cfgparse.c: use after free suspected by coverity #1563
Labels
status: fixed
This issue is a now-fixed bug.
type: code-report
This issue describes a code report (like valgrind or coverity)
Comments
chipitsine
added
the
type: code-report
|
Indeed, it seems there is an issue here. For me, when diff --git a/src/cfgparse.c b/src/cfgparse.c
index b1ec46f2f..2f886d92e 100644
--- a/src/cfgparse.c
+++ b/src/cfgparse.c
@@ -1862,6 +1862,7 @@ int readcfgfile(const char *file)
file, linenum);
err_code |= ERR_ALERT | ERR_FATAL;
fatal++;
+ outlinesize = 0;
goto next_line;
}
/* try again */I replaced |
haproxy-mirror
pushed a commit
that referenced
this issue
May 19, 2022
…dcfgfile() When the line parsing failed because outline buffer must be reallocated, if my_realloc2() call fails, the buffer size must be reset. Indeed, in this case the current line is skipped, a fatal error is reported and we jump to the next line. At this stage the outline buffer is NULL. If the buffer size is not reset, the next call to parse_line() crashes because we try to write in the buffer. We fail to detect the outline buffer is too small to copy any character. To fix the issue, outlinesize variable must be set to 0 when outline allocation failed. This patch should fix the issue #1563. It must be backported as far as 2.2.
|
It should be fixed now. |
capflam
added
status: fixed
|
I'll check coverity tomorrow (after next scheduled scan). |
|
coverity thinks it is still unresolved
|
|
Ok, thanks. I'll review it again. |
capflam
added
dev
|
current detection log |
haproxy-mirror
pushed a commit
that referenced
this issue
May 20, 2022
In issue #1563, Coverity reported a very interesting issue about a possible UAF in the config parser if the config file ends in with a very large line followed by an empty one and the large one causes an allocation failure. The issue essentially is that we try to go on with the next line in case of allocation error, while there's no point doing so. If we failed to allocate memory to read one config line, the same may happen on the next one, and blatantly dropping it while trying to parse what follows it. In the best case, subsequent errors will be incorrect due to this prior error (e.g. a large ACL definition with many patterns, followed by a reference of this ACL). Let's just immediately abort in such a condition where there's no recovery possible. This may be backported to all versions once the issue is confirmed to be addressed. Thanks to Ilya for the report.
|
Thank you Ilya, this one is valid and interesting in addition :-) |
|
Let's check tomorrow
…On Fri, May 20, 2022, 12:27 PM Willy Tarreau ***@***.***> wrote:
Thank you Ilya, this one is valid and interesting in addition :-)
I think 8ec9c81
<8ec9c81>
should address it now.
—
Reply to this email directly, view it on GitHub
<#1563 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQ5KUFLGWO7J3QKZH64QRDVK45HDANCNFSM5O6GMYPA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
coverity confirms it is fixed |
|
Cool, thanks Ilya! |
capflam
added
status: fixed
FireBurn
pushed a commit
to FireBurn/haproxy
that referenced
this issue
Jun 8, 2022
…dcfgfile() When the line parsing failed because outline buffer must be reallocated, if my_realloc2() call fails, the buffer size must be reset. Indeed, in this case the current line is skipped, a fatal error is reported and we jump to the next line. At this stage the outline buffer is NULL. If the buffer size is not reset, the next call to parse_line() crashes because we try to write in the buffer. We fail to detect the outline buffer is too small to copy any character. To fix the issue, outlinesize variable must be set to 0 when outline allocation failed. This patch should fix the issue haproxy#1563. It must be backported as far as 2.2. (cherry picked from commit dfe32c7) Signed-off-by: Christopher Faulet <cfaulet@haproxy.com> (cherry picked from commit 693e644) Signed-off-by: Christopher Faulet <cfaulet@haproxy.com> (cherry picked from commit 6ae99cb) Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
FireBurn
pushed a commit
to FireBurn/haproxy
that referenced
this issue
Jun 8, 2022
In issue haproxy#1563, Coverity reported a very interesting issue about a possible UAF in the config parser if the config file ends in with a very large line followed by an empty one and the large one causes an allocation failure. The issue essentially is that we try to go on with the next line in case of allocation error, while there's no point doing so. If we failed to allocate memory to read one config line, the same may happen on the next one, and blatantly dropping it while trying to parse what follows it. In the best case, subsequent errors will be incorrect due to this prior error (e.g. a large ACL definition with many patterns, followed by a reference of this ACL). Let's just immediately abort in such a condition where there's no recovery possible. This may be backported to all versions once the issue is confirmed to be addressed. Thanks to Ilya for the report. (cherry picked from commit 8ec9c81) Signed-off-by: Christopher Faulet <cfaulet@haproxy.com> (cherry picked from commit dbcd9db) Signed-off-by: Christopher Faulet <cfaulet@haproxy.com> (cherry picked from commit 0c68be7) Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
FireBurn
pushed a commit
to FireBurn/haproxy
that referenced
this issue
Jun 8, 2022
…dcfgfile() When the line parsing failed because outline buffer must be reallocated, if my_realloc2() call fails, the buffer size must be reset. Indeed, in this case the current line is skipped, a fatal error is reported and we jump to the next line. At this stage the outline buffer is NULL. If the buffer size is not reset, the next call to parse_line() crashes because we try to write in the buffer. We fail to detect the outline buffer is too small to copy any character. To fix the issue, outlinesize variable must be set to 0 when outline allocation failed. This patch should fix the issue haproxy#1563. It must be backported as far as 2.2. (cherry picked from commit dfe32c7) Signed-off-by: Christopher Faulet <cfaulet@haproxy.com> (cherry picked from commit 693e644) Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
FireBurn
pushed a commit
to FireBurn/haproxy
that referenced
this issue
Jun 8, 2022
In issue haproxy#1563, Coverity reported a very interesting issue about a possible UAF in the config parser if the config file ends in with a very large line followed by an empty one and the large one causes an allocation failure. The issue essentially is that we try to go on with the next line in case of allocation error, while there's no point doing so. If we failed to allocate memory to read one config line, the same may happen on the next one, and blatantly dropping it while trying to parse what follows it. In the best case, subsequent errors will be incorrect due to this prior error (e.g. a large ACL definition with many patterns, followed by a reference of this ACL). Let's just immediately abort in such a condition where there's no recovery possible. This may be backported to all versions once the issue is confirmed to be addressed. Thanks to Ilya for the report. (cherry picked from commit 8ec9c81) Signed-off-by: Christopher Faulet <cfaulet@haproxy.com> (cherry picked from commit dbcd9db) Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
FireBurn
pushed a commit
to FireBurn/haproxy
that referenced
this issue
Jun 8, 2022
…dcfgfile() When the line parsing failed because outline buffer must be reallocated, if my_realloc2() call fails, the buffer size must be reset. Indeed, in this case the current line is skipped, a fatal error is reported and we jump to the next line. At this stage the outline buffer is NULL. If the buffer size is not reset, the next call to parse_line() crashes because we try to write in the buffer. We fail to detect the outline buffer is too small to copy any character. To fix the issue, outlinesize variable must be set to 0 when outline allocation failed. This patch should fix the issue haproxy#1563. It must be backported as far as 2.2. (cherry picked from commit dfe32c7) Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
FireBurn
pushed a commit
to FireBurn/haproxy
that referenced
this issue
Jun 8, 2022
In issue haproxy#1563, Coverity reported a very interesting issue about a possible UAF in the config parser if the config file ends in with a very large line followed by an empty one and the large one causes an allocation failure. The issue essentially is that we try to go on with the next line in case of allocation error, while there's no point doing so. If we failed to allocate memory to read one config line, the same may happen on the next one, and blatantly dropping it while trying to parse what follows it. In the best case, subsequent errors will be incorrect due to this prior error (e.g. a large ACL definition with many patterns, followed by a reference of this ACL). Let's just immediately abort in such a condition where there's no recovery possible. This may be backported to all versions once the issue is confirmed to be addressed. Thanks to Ilya for the report. (cherry picked from commit 8ec9c81) Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
capflam
removed
2.2
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Tool Name and Version
coverity
Code Report
Additional Information
No response
Output of
haproxy -vvThe text was updated successfully, but these errors were encountered: