Opened 3 years ago

Closed 3 years ago

#19223 closed defect (fixed)

Potential heap corruption in do_getpass in routerkeys.c

Reported by: asn Owned by:
Priority: Low Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: tor-bug-bounty, 028-backport, isaremoved, nickwants029, review-group-10
Cc: Actual Points:
Parent ID: Points: 0.5
Reviewer: Sponsor:

Description

Hello,

this is a bug by Guido Vranken from our bug bounty program. This bug is not triggerable in the current codebase, but it's still a good idea to fix, for future safety.

Here follows the bug report as received:


do_getpass contains this code:

  if (twice) {
    const char msg[] = "One more time:";
    size_t p2len = strlen(prompt) + 1;
    if (p2len < sizeof(msg))
      p2len = sizeof(msg);
    prompt2 = tor_malloc(strlen(prompt)+1);
    memset(prompt2, ' ', p2len);
    memcpy(prompt2 + p2len - sizeof(msg), msg, sizeof(msg));

    buf2 = tor_malloc_zero(buflen);
  }

There is only one call to this function in the code for which twice == 1:

  if (do_getpass("Enter new passphrase:", pwbuf0, sizeof(pwbuf0), 1,
                 get_options()) < 0) {
    log_warn(LD_OR, "NO/failed passphrase");
    return -1;
  }

This will not trigger a memory corruption, but if the first parameter had been shorter, it would:

Compile and run like this:

$ gcc -fomit-frame-pointer -fsanitize=address do_getpass.c 
$ ./a.out "Enter new passphrase:"
$ ./a.out "Enter new passphrase"
$ ./a.out "Enter new passphras"
$ ./a.out "Enter new passphra"
$ ./a.out "Enter new passphr"
$ ./a.out "Enter new passph"
$ ./a.out "Enter new passp"
$ ./a.out "Enter new pass"
$ ./a.out "Enter new pas"

==7883== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60040000dffe at pc 0x400c0a bp 0x7fff8d9c22e0 sp 0x7fff8d9c22d8
...
...

So it's not really a vulnerability at present, but I thought I'd mention it to
you since it struck me as odd and it could become a problem if you pass a
dynamic, potentially short string (for ex. created with snprintf) to
do_getpass.

Child Tickets

Change History (8)

comment:1 Changed 3 years ago by nickm

Keywords: 028-backport added
Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

comment:2 Changed 3 years ago by isabela

Keywords: isaremoved added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

comment:3 Changed 3 years ago by nickm

Keywords: nickwants029 added

This would IMO be even less than .5 points to fix, and it implies a potential security issue down the road if we don't. (We would be kicking ourselves if this ever bit us.)

comment:4 Changed 3 years ago by nherring

Have a suggested fix, but don't know the model for adding tests, code review, submission, etc. Ptr to FAQ/instructions appreciated.

$ git diff src/or/routerkeys.c
diff --git a/src/or/routerkeys.c b/src/or/routerkeys.c
index 060ffd8..d5e7051 100644
--- a/src/or/routerkeys.c
+++ b/src/or/routerkeys.c
@@ -48,8 +48,8 @@ do_getpass(const char *prompt, char *buf, size_t buflen,
     size_t p2len = strlen(prompt) + 1;
     if (p2len < sizeof(msg))
       p2len = sizeof(msg);
-    prompt2 = tor_malloc(strlen(prompt)+1);
-    memset(prompt2, ' ', p2len);
+    prompt2 = tor_malloc(p2len);
+    memset(prompt2, ' ', p2len - sizeof(msg));
     memcpy(prompt2 + p2len - sizeof(msg), msg, sizeof(msg));

     buf2 = tor_malloc_zero(buflen);

comment:5 Changed 3 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.9.x-final
Status: newneeds_review

Hi! Ive marked this for review, and since it's small, I've marked it for potential inclusion in 0.2.9.

For more information about tests, code review, submission, etc, look at the doc/HACKING subdirectory of the Tor source tree, especially doc/HACKING/GettingStarted.md

Thanks!

comment:6 Changed 3 years ago by nickm

Keywords: review-group-10 added

Add needs_review 0.2.9 tickets, plus ones that have been in needs_revision for less than a week, to review-group-10.

comment:7 Changed 3 years ago by asn

Status: needs_reviewmerge_ready

Hello,

I reviewed nherring's patch and it seems alright. I also tested it against Guido's PoC and ASAN does not crash anymore.

BTW, since no branch was provided, I pushed nherring's patch on my repo as bug19223 and also added a changes file. Please check it out.

comment:8 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

lgtm; merged!

Note: See TracTickets for help on using tickets.