Validate verification code input before sending it #103
Conversation
This makes sure that the verification code only contains characters a-z A-Z
0-9, and is of limited length.
Otherwise it is possible for a user to cause arbitrary data to be sent to the
server, including invalid JSON. This could be a problem if the JSON parser on
the receiving end has bugs.
For example, code input 'A"}BBBBBBBB...' would lead to JSON data
'{"session_id":"SESSIONID","pin":"A"}BBBBBBBB..."}' to be sent to the server.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #103 +/- ##
==========================================
+ Coverage 71.02% 72.87% +1.85%
==========================================
Files 6 6
Lines 283 306 +23
Branches 44 47 +3
==========================================
+ Hits 201 223 +22
Misses 71 71
- Partials 11 12 +1 ☔ View full report in Codecov by Sentry. |
Hooray for unit tests!
|
When using pamtester to test the PR, I get a segmentation fault on and empty code response: It would be nice if you could extend the validation to the username input and return a |
|
I fixed the segfault - I overlooked that tty_input() returns NULL on empty input. This can also bite the group input, I opened PR #104 for this. I will extent this to cover user names in a separate |
| @@ -77,6 +77,35 @@ char *tty_input(pam_handle_t *pamh, const char *text, int echo_code) | |||
| return ret; | |||
| } | |||
|
|
|||
| int input_is_safe(const char *input, size_t max_length) | |||
| { | |||
| size_t length = strlen(input); | |||
There was a problem hiding this comment.
This is not safe. If input isn't terminated by a \0, strlen() will run into unrelated memory. This is unlikely but could possibly occur.
The better solution is to use strnlen() and then make sure that the string is 0-terminated. Or use strndup() to make a local copy of the string.
| @@ -77,6 +77,35 @@ char *tty_input(pam_handle_t *pamh, const char *text, int echo_code) | |||
| return ret; | |||
| } | |||
|
|
|||
| int input_is_safe(const char *input, size_t max_length) | |||
There was a problem hiding this comment.
If you intend to return a true/false value, please use an explicit bool value for that.
This makes sure that the verification code only contains characters a-z A-Z 0-9, and is of limited length.
Otherwise it is possible for a user to cause arbitrary data to be sent to the server, including invalid JSON. This could be a problem if the JSON parser on the receiving end has bugs.
For example, code input 'A"}BBBBBBBB...' would lead to JSON data '{"session_id":"SESSIONID","pin":"A"}BBBBBBBB..."}' to be sent to the server.