Opened 4 years ago

Closed 4 years ago

#16823 closed defect (fixed)

extra tor_free() for create_cell_t in command_process_create_cell()

Reported by: isis Owned by:
Priority: Low Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.8-alpha
Severity: Keywords: tor-relay, tor-guard
Cc: isis, yawning, nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In command_process_create_cell() (on master, as of commit da04fed865b6df09b33e6b632d51d34b3eb20d14)

     memset(&created_cell, 0, sizeof(created_cell));
     len = onion_skin_server_handshake(ONION_HANDSHAKE_TYPE_FAST,
                                       create_cell->onionskin,
                                       create_cell->handshake_len,
                                       NULL,
                                       created_cell.reply,
                                       keys, CPATH_KEY_MATERIAL_LEN,
                                       rend_circ_nonce);
     tor_free(create_cell);
     if (len < 0) {
       log_warn(LD_OR,"Failed to generate key material. Closing.");
       circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_INTERNAL);
       tor_free(create_cell);
       return;
     }

Which is a double-free (somewhat dependent on what the PREDICT_LIKELY macro generates).

I haven't tested yet, but it might be possible to crash relays with this bug. We should probably patch this ASAP.

Child Tickets

Change History (10)

comment:1 Changed 4 years ago by isis

Status: newneeds_review

A fix is in my bug16823 branch.

comment:2 Changed 4 years ago by yawning

Keywords: 024-backport 025-backport 026-backport added
Priority: normalblocker
Version: Tor: 0.2.4.10-alpha

We discussed this on #tor-dev, unfortunately. Nice find. Bumping up the priority, and marking for backport.

ACK the branch, since it's as discussed. Needs a changes file but nickm or I can write one if you don't feel like it.

comment:3 Changed 4 years ago by nickm

Good catch, but not a real bug at all, I think. Remember the definition of tor_free:

/** Release memory allocated by tor_malloc, tor_realloc, tor_strdup, etc.
 * Unlike the free() function, tor_free() will still work on NULL pointers,
 * and it sets the pointer value to NULL after freeing it.
 *
 * This is a macro.  If you need a function pointer to release memory from
 * tor_malloc(), use tor_free_().
 */
#define tor_free(p) STMT_BEGIN                                 \
    if (PREDICT_LIKELY((p)!=NULL)) {                           \
      free(p);                                                 \
      (p)=NULL;                                                \
    }                                                          \
  STMT_END

So the first tor_free will set create_cell to NULL, and the second tor_free will do nothing.

So unless I'm missing something big, this is a programming mistake, but not actually exploitable. Please let me know if I'm wrong, or downgrade to "normal priority, no backport" if I'm right?

Thanks!

comment:4 Changed 4 years ago by nickm

PREDICT_LIKELY() expands to:

/** Macro: Evaluates to <b>exp</b> and hints the compiler that the value
 * of <b>exp</b> will probably be true.
 *
 * In other words, "if (PREDICT_LIKELY(foo))" is the same as "if (foo)",
 * except that it tells the compiler that the branch will be taken most of the
 * time.  This can generate slightly better code with some CPUs.
 */
#define PREDICT_LIKELY(exp) __builtin_expect(!!(exp), 1)

comment:5 in reply to:  3 Changed 4 years ago by yawning

Keywords: security 024-backport 025-backport 026-backport removed
Milestone: Tor: 0.2.7.x-final
Priority: blockernormal

Replying to nickm:

So unless I'm missing something big, this is a programming mistake, but not actually exploitable. Please let me know if I'm wrong, or downgrade to "normal priority, no backport" if I'm right?

Yeah I think baring code generation weirdness, you're right, sorry for the confusion, my bad. We still should fix this though, since it looks hair raising.

comment:6 in reply to:  2 Changed 4 years ago by isis

Keywords: security 024-backport 025-backport 026-backport added
Milestone: Tor: 0.2.7.x-final
Priority: normalblocker

Replying to yawning:

We discussed this on #tor-dev, unfortunately. Nice find. Bumping up the priority, and marking for backport.

ACK the branch, since it's as discussed. Needs a changes file but nickm or I can write one if you don't feel like it.


I added a changes file to my branch, but I tagged it Major bugfixes (security, relay) so, if it's not actually a security bug, then we should remove the security tag. Or just rewrite the thing however it should be written, with whatever classifications are appropriate.

comment:7 in reply to:  4 Changed 4 years ago by isis

Replying to nickm:

PREDICT_LIKELY() expands to:

/** Macro: Evaluates to <b>exp</b> and hints the compiler that the value
 * of <b>exp</b> will probably be true.
 *
 * In other words, "if (PREDICT_LIKELY(foo))" is the same as "if (foo)",
 * except that it tells the compiler that the branch will be taken most of the
 * time.  This can generate slightly better code with some CPUs.
 */
#define PREDICT_LIKELY(exp) __builtin_expect(!!(exp), 1)


Yeah, you're right, it should be okay. Phew! Sorry for tagging it security in a panic.

comment:8 Changed 4 years ago by yawning

Keywords: security 024-backport 025-backport 026-backport removed
Milestone: Tor: 0.2.7.x-final
Priority: blockernormal
Version: Tor: 0.2.4.10-alphaTor: 0.2.4.8-alpha

Trac just pooped over my tag changes. ;_;

Since it's just a cleanup, we probably don't even need a changes file at this point.

comment:9 Changed 4 years ago by isis

Summary: potential double-free in command_process_create_cell()extra tor_free() for create_cell_t in command_process_create_cell()

comment:10 Changed 4 years ago by nickm

Priority: normalminor
Resolution: fixed
Status: needs_reviewclosed

I've revised the commit message and cherry-picked the fix. Thanks!

Note: See TracTickets for help on using tickets.