fuzzing: fixed harness bug #1393
Conversation
pkillarjun
force-pushed
the
harness-bug
branch
from
3f71ceb
to
69b830b
Compare
|
Hmm, does |
BTW, can we see the unit.log when running fuzzing? If yes, it's a good way to see what the value from "addr" is. |
You could take a look at its implementation, before |
I think you are referring to the if (addr->length == 0) {
nxt_thread_log_error(NXT_LOG_ERR, "socket address cannot be empty");
return NULL;
}
static nxt_sockaddr_t *
nxt_sockaddr_inet_parse(nxt_mp_t *mp, nxt_str_t *addr)
{
u_char *p;
size_t length;
nxt_int_t port;
in_addr_t inaddr;
nxt_sockaddr_t *sa;
p = memchr(addr->start, ':', addr->length);
if (p == NULL) {
length = addr->length;
} else {
length = p - addr->start; // This is where the length is assigned.
}
inaddr = INADDR_ANY;So, we did check for Because the length is 0, a bug in if (nxt_slow_path(*(buf + length - 1) == '.')) { // ASan says hello crash.
return INADDR_NONE;
} |
Patch for enabling logging:diff --git a/fuzzing/nxt_json_fuzz.c b/fuzzing/nxt_json_fuzz.c
index fa222988..eafbbad8 100644
--- a/fuzzing/nxt_json_fuzz.c
+++ b/fuzzing/nxt_json_fuzz.c
@@ -33,6 +33,7 @@ LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
{
nxt_mp_t *mp;
nxt_str_t input;
+ nxt_task_t task;
nxt_thread_t *thr;
nxt_runtime_t *rt;
nxt_conf_value_t *conf;
@@ -43,7 +44,11 @@ LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
return 0;
}
+ nxt_main_log.level = NXT_LOG_DEBUG;
+ task.log = &nxt_main_log;
+
thr = nxt_thread();
+ thr->task = &task;
mp = nxt_mp_create(1024, 128, 256, 32);
if (mp == NULL) {I think this is what you are asking for: |
|
Hi @pkillarjun, |
src/nxt_sockaddr.c
Outdated
| @@ -734,6 +734,11 @@ nxt_sockaddr_inet_parse(nxt_mp_t *mp, nxt_str_t *addr) | |||
|
|
|||
| inaddr = INADDR_ANY; | |||
|
|
|||
| if (length == 0) { | |||
| nxt_thread_log_error(NXT_LOG_ERR, "invalid size \"%V\"", length); | |||
There was a problem hiding this comment.
The logging could be like this:
(..., "invalid address \"%V\"", addr);
There was a problem hiding this comment.
The length check should really come before inaddr = INADDR_ANY;... but lets see if this is really the right place for this...
|
Also, 1. Yes there is a typo: nxt_thread_log_error(NXT_LOG_ERR, "invalid size \"%V\"", length); /* this looks like a typo, %V is used for nxt_str_t. */2. Yes, commit message is super misleading.I will fix them.
Ok, I will. |
There was a problem hiding this comment.
json: fix Heap-buffer-overflow bug
I don't think the prefix is "json:", you could just use like this:
Fix Heap-buffer-overflow bug in nxt_sockaddr_inet_parse()
Others look good to me.
@ac000 welcome to review.
|
Hmm, I think I'm still confused about exactly what causes the issue... {
"listeners": {
":": {
"pass": "routes"
}
},
"routes": [
{
"match": {
"uri": "*"
},
"action": {
"share": "/srv/unit-share$uri"
}
}
]
} |
|
An address of if (length != 1 || addr->start[0] != '*') {
inaddr = nxt_inet_addr(addr->start, length);
if (nxt_slow_path(inaddr == INADDR_NONE)) {
nxt_thread_log_error(NXT_LOG_ERR, "invalid address \"%V\"", addr);
return NULL;
}
}in |
|
If we call into If we find if (p == NULL) {
length = addr->length;
} else {
length = p - addr->start;
}in Then yes, I believe returning "invalid address" is right, not because of any error, but just because there's no point going any further. However couldn't we short circuit this whole thing by simply adding a The port needs to be specified, so I believe the shortest valid listener address would be six characters, e.g |
|
I've tried all kinds of crazzy stuff and don't see any crash... {
"error": "Invalid configuration.",
"detail": "The listener address \":::\" is invalid."
}
{
"error": "Invalid configuration.",
"detail": "The listener address \"1:1234\" is invalid."
}
{
"error": "Invalid configuration.",
"detail": "The listener address \"1:1\" is invalid."
}
{
"error": "Invalid configuration.",
"detail": "The listener address \"1:\" is invalid."
}
{
"error": "Invalid configuration.",
"detail": "The listener address \":1:\" is invalid."
}
{
"error": "Invalid configuration.",
"detail": "The listener address \": \" is invalid."
}
{
"error": "Invalid configuration.",
"detail": "The listener address \" :\" is invalid."
}
{
"error": "Invalid configuration.",
"detail": "The listener address \" :\" is invalid."
}
{
"error": "Invalid configuration.",
"detail": "The listener address \" : \" is invalid."
} |
|
Trying to parse the commit message
addr->start is just some pointer that should be pointing to the start of the address string.
The only way I can get p = memchr(addr->start, ':', addr->length);But then in that case (in end = buf + length;means Which then means we don't do while (buf < end) {
...or if (dots == 3 && octet < 256) {
...and so just return INADDR_NONE;AFAICT, with a |
OK, I see it. I think the commit message is a little misleading as I don't see any crash. Could also be fixed with (in if (nxt_slow_path(length > 0 && *(buf + length - 1) == '.')) {
return INADDR_NONE;
}Though I guess your patch is better as it's probably best to invalidate the address as early as possible... But I think the commit message needs re-working to make it much clearer what and where the problem is... |
|
Perhaps something like btw I was curious what GCC's -fanalyzer would say about this sort of thing... |
Agree, it looks better to be in |
Try this, Total number of space is 256: {"listeners":{": ":""}}You know I still think our patch is wrong, and there is some kind of weird bug in JSON parsing. |
|
Good point, will look into JSON parsing. |
Did you mean your example is an invalid JSON? |
No, not that. When the JSON parser assigns The JSON parser rejects everything if it goes under HEX Or possibly I am thinking of something else here. |
It looks like another topic related to JSON parsing. |
Ha, make sense now.
I was thinking of something else. I meant to say: The address is nothing, the port is not even a number. So why are these even assigned? {
"listeners": {
": "
:""
}
} |
|
Firstly, it’s valid JSON. It’s successful parsing by JSON parser, then JSON validator which is independent of JSON parser validates the parsed object. The sockaddr parse happens during validation. |
Oh, shit. |
pkillarjun
force-pushed
the
harness-bug
branch
from
0d581e8
to
1e4c425
Compare
|
No worries, that kind of confusion is common. The address is used as an object key. |
|
Alright, now we are on the same page. You can take a look at it. 1e4c425 |
LGTM. |
I don't think adding a Python test is necessary. Cifuzz will take care of every malformed data. For Trailing space, I need to use
Alright, that will do. |
Ok, we could do it separately. |
|
Right, from what I've seen that test likely wouldn't show any issue, unless the byte before addr->start just so happened to be a |
|
Thanks @pkillarjun looks good, I'll be happy to merge this on Monday... |
False positive bug in harness due to improper use of the internal API. Fixes: a93d878 ("fuzzing: add fuzzing targets") Signed-off-by: Arjun <[email protected]> Signed-off-by: Andrew Clayton <[email protected]>
This was found via ASan.
Given a listener address like ":" (or any address where the first
character is a colon) we can end up under-reading the addr->start
buffer here
if (nxt_slow_path(*(buf + length - 1) == '.')) {
due to length (essentially the position of the ":" in the string) being
0.
Seeing as any address that starts with a ":" is invalid Unit config
wise, we should simply reject the address if length == 0 in
nxt_sockaddr_inet_parse().
Link: <https://clang.llvm.org/docs/AddressSanitizer.html>
Signed-off-by: Arjun <[email protected]>
[ Commit message - Andrew ]
Signed-off-by: Andrew Clayton <[email protected]>
|
Rebased with master |
The first patch is straightforward; 69b830b 70961.
The second patch, on the other hand, is quite suspicious. It works now, but I have no idea what I am doing.
I think it does require some changes to the JSON parsing and JSON after parsing.
0d581e8 70961
Report:
The
addr->lengthis0. It does make my job easy, so I added a check: if the length is zero, return null.But
addr->startholds 303 bytes.Bytes in
addr->startxxd clusterfuzz-testcase-minimized-fuzz_json-4875165201137664I believe the second patch does require some fine-tuning, nonetheless.