Fix log injection attack and return on JSON parse failure #957
Conversation
cdecker
changed the title
|
I guess if we indeed have a shell escape character in a string, e.g., invoice description, that'll trigger an invalid token error? In that case, I'll move the check to |
|
Removed check for unprintable characters in JSON, replaced with string sanitization in |
lightningd/log.c
Outdated
| @@ -289,6 +295,12 @@ void logv_add(struct log *log, const char *fmt, va_list ap) | |||
| list_del_from(&log->lr->log, &l->list); | |||
|
|
|||
| tal_append_vfmt(&l->log, fmt, ap); | |||
|
|
|||
| /* Sanitize any non-printable characters, and replace with '?' */ | |||
| for (size_t i=0; i<strlen(l->log); i++) | |||
There was a problem hiding this comment.
You can iterate from oldlen, not 0, BTW, since this is a simple append...
lightningd/log.c
Outdated
|
|
||
| /* Sanitize any non-printable characters, and replace with '?' */ | ||
| for (size_t i=0; i<strlen(l->log); i++) | ||
| if (l->log[i] < ' ' || l->log[i] >= 0x7f) |
There was a problem hiding this comment.
Should we be checking == 0x7f so we let through UTF-8? That would seem sensible...
There was a problem hiding this comment.
As much as I'd like to have UTF-8 support in here, that'd mean we now need to carry context and have multi-char replacements in some cases, e.g., there are multi-byte control characters again.
As reported by @practicalswift in ElementsProject#945 it is possible to inject non-printable, or shell escape, characters in a json command, that will fail to parse and then clear the shell. Reported-by: @practicalswift Signed-off-by: Christian Decker <decker.christian@gmail.com>
The JSON-RPC spec specifies that if the request is unparseable we should return an error with a NULL id. This is a bit more friendly than slamming the door in the face. Signed-off-by: Christian Decker <decker.christian@gmail.com>
|
Rebased and started sanitization from ACK 5267456 |
|
Post-merge ACK 5267456 Sorry for the late review :-) |
As reported by @practicalswift in #945 it is possible to inject
non-printable, or shell escape, characters in a json command, that
will fail to parse and then clear the shell.
The second commit also adds an error response when we're unable
to parse the request, just to be extra nice 😉
Fixes #945
Fixes #908