Florian Obser <[email protected]> wrote:
> On 2022-05-02 03:04 +0430, Ali Farzanrad <[email protected]> wrote:
> > Hi tech@,
> >
> > I know that acme-client is unveiled properly, but isn't it better to
> > check token names?
>
> Nice catch, the token is untrusted input.
> We should validate this differently though.
>
> RFC 8555, 8.5 HTTP Challenge:
>
> token (required, string): A random value that uniquely identifies
> the challenge. This value MUST have at least 128 bits of entropy.
> It MUST NOT contain any characters outside the base64url alphabet
> and MUST NOT include base64 padding characters ("=").
>
> base64url is defined in
> RFC 4648, 5. Base 64 Encoding with URL and Filename Safe Alphabet
>
> It's basically isalpha || '-' || '_'.
>
> Are you up to implementing that check instead?
>
Hi Florian,
Yes, I read the RFC, it should work, but I couldn't test it yet, because
my domain manager is a little lazy (I've registeret 2 subdomains for my
domain, but it is not listed in name servers yet). I'll probably test
it tomorrow.
I read the isalnum man page, it said that isalnum might have different
behavior based on locale, so I decide to not use that. Is it OK?
Index: chngproc.c
===================================================================
RCS file: /cvs/src/usr.sbin/acme-client/chngproc.c,v
retrieving revision 1.16
diff -u -p -r1.16 chngproc.c
--- chngproc.c 12 Jul 2021 15:09:20 -0000 1.16
+++ chngproc.c 3 May 2022 13:03:45 -0000
@@ -26,6 +26,8 @@
#include "extern.h"
+static int is_base64url(const char *);
+
int
chngproc(int netsock, const char *root)
{
@@ -77,6 +79,10 @@ chngproc(int netsock, const char *root)
goto out;
else if ((tok = readstr(netsock, COMM_TOK)) == NULL)
goto out;
+ else if (!is_base64url(tok)) {
+ warnx("token is not base64url");
+ goto out;
+ }
if (asprintf(&fmt, "%s.%s", tok, th) == -1) {
warn("asprintf");
@@ -152,4 +158,24 @@ out:
free(th);
free(tok);
return rc;
+}
+
+static int
+is_base64url(const char *s)
+{
+ for (; *s; ++s) {
+ int ch = (unsigned char)*s;
+ if ((ch >= '0' && ch <= '9') ||
+ (ch >= 'A' && ch <= 'Z') ||
+ (ch >= 'a' && ch <= 'z'))
+ continue;
+ switch (ch) {
+ case '-':
+ case '_':
+ break;
+ default:
+ return 0;
+ }
+ }
+ return 1;
}
