Skip to content

Commit

Permalink
Validate verification code input before sending it
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Martin Lambers committed Oct 17, 2024
1 parent 5401b77 commit 6027aa2
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 0 deletions.
7 changes: 7 additions & 0 deletions src/pam_weblogin.c
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,13 @@ PAM_EXTERN int pam_sm_authenticate(pam_handle_t *pamh, UNUSED int flags, int arg
++retry)
{
char *code = tty_input(pamh, PROMPT_CODE, PAM_PROMPT_ECHO_OFF);
if (!input_is_safe(code, MAX_INPUT_LENGTH))
{
tty_output(pamh, "invalid code");
log_message(LOG_ERR, "invalid code");
free(code);
continue;
}

/* Prepare URL... */
url = str_printf("%s/%s", cfg->url, API_CHECK_CODE_PATH);
Expand Down
2 changes: 2 additions & 0 deletions src/pam_weblogin.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
#define PROMPT_GROUP "\nSelect group: "
#define PROMPT_WRONG_NUMBER "Wrong number"

#define MAX_INPUT_LENGTH 256

#ifndef GIT_COMMIT
#define GIT_COMMIT 0000
#endif
Expand Down
18 changes: 18 additions & 0 deletions src/tty.c
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,24 @@ 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)

Check warning on line 80 in src/tty.c

View check run for this annotation

Codecov / codecov/patch

src/tty.c#L80

Added line #L80 was not covered by tests
{
for (size_t i = 0; input[i]; i++)
{
if (i > max_length
/* Don't use isalnum() here because it is locale-dependent,
* and don't use isalnum_l() because it is not portable.
* Instead, hardcode a check for ASCII a-z A-Z 0-9. */
|| !( (input[i] >= 'a' && input[i] <= 'z')
|| (input[i] >= 'A' && input[i] <= 'Z')
|| (input[i] >= '0' && input[i] <= '9')))

Check warning on line 90 in src/tty.c

View check run for this annotation

Codecov / codecov/patch

src/tty.c#L88-L90

Added lines #L88 - L90 were not covered by tests
{
return 0;

Check warning on line 92 in src/tty.c

View check run for this annotation

Codecov / codecov/patch

src/tty.c#L92

Added line #L92 was not covered by tests
}
}
return 1;

Check warning on line 95 in src/tty.c

View check run for this annotation

Codecov / codecov/patch

src/tty.c#L95

Added line #L95 was not covered by tests
}

void tty_output(pam_handle_t *pamh, const char *text)
{
/* note: on MacOS, pam_message.msg is a non-const char*, so we need to copy it */
Expand Down
2 changes: 2 additions & 0 deletions src/tty.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#define TTY_H

#include <syslog.h>
#include <stddef.h>

#include <security/pam_appl.h>
#include <security/pam_modules.h>
Expand All @@ -10,6 +11,7 @@ void log_message(int priority, const char *fmt, ...)
__attribute__ ((format (printf, 2, 3)));

char *tty_input(pam_handle_t *pamh, const char *text, int echo_code);
int input_is_safe(const char *input, size_t max_length);
void tty_output(pam_handle_t *pamh, const char *text);

#endif /* TTY_H */

0 comments on commit 6027aa2

Please sign in to comment.