1. 11 Jun, 2016 1 commit
    • Nick Mathewson's avatar
      Enable -Wnull-dereference (GCC >=6.1), and fix the easy cases · 4f8086fb
      Nick Mathewson authored
      This warning, IIUC, means that the compiler doesn't like it when it
      sees a NULL check _after_ we've already dereferenced the
      variable. In such cases, it considers itself free to eliminate the
      NULL check.
      There are a couple of tricky cases:
      One was the case related to the fact that tor_addr_to_in6() can
      return NULL if it gets a non-AF_INET6 address.  The fix was to
      create a variant which asserts on the address type, and never
      returns NULL.
  2. 28 May, 2016 1 commit
  3. 27 May, 2016 10 commits
  4. 26 May, 2016 5 commits
  5. 25 May, 2016 13 commits
    • Nick Mathewson's avatar
      Merge branch 'maint-0.2.8' · b7fac185
      Nick Mathewson authored
    • Nick Mathewson's avatar
    • Nick Mathewson's avatar
      Merge branch 'maint-0.2.8' · 28cbcd03
      Nick Mathewson authored
    • Nick Mathewson's avatar
    • Nick Mathewson's avatar
      Fix a double-free bug in routerlist_reparse_old · 9cf6af76
      Nick Mathewson authored
      I introduced this bug when I moved signing_key_cert into
      signed_descriptor_t. Bug not in any released Tor.  Fixes bug 19175, and
      another case of 19128.
      Just like signed_descriptor_from_routerinfo(), routerlist_reparse_old()
      copies the fields from one signed_descriptor_t to another, and then
      clears the fields from the original that would have been double-freed by
      freeing the original.  But when I fixed the s_d_f_r() bug [#19128] in
      50cbf220, I missed the fact that the code was duplicated in
      Duplicated code strikes again!
      For a longer-term solution here, I am not only adding the missing fix to
      r_p_o(): I am also extracting the duplicated code into a new function.
      Many thanks to toralf for patiently sending me stack traces until
      one made sense.
    • Nick Mathewson's avatar
      Merge branch 'maint-0.2.8' · 44ea3dc3
      Nick Mathewson authored
    • Nick Mathewson's avatar
    • Nick Mathewson's avatar
      Merge branch 'maint-0.2.8' · a3ec811c
      Nick Mathewson authored
    • Nick Mathewson's avatar
    • Nick Mathewson's avatar
      Fix a dangling pointer issue in our RSA keygen code · c4c4380a
      Nick Mathewson authored
      If OpenSSL fails to generate an RSA key, do not retain a dangling
      pointer to the previous (uninitialized) key value. The impact here
      should be limited to a difficult-to-trigger crash, if OpenSSL is
      running an engine that makes key generation failures possible, or if
      OpenSSL runs out of memory. Fixes bug 19152; bugfix on Found by Yuan Jochen Kang, Suman Jana, and Baishakhi
      This is potentially scary stuff, so let me walk through my analysis.
      I think this is a bug, and a backport candidate, but not remotely
      triggerable in any useful way.
      Observation 1a:
      Looking over the OpenSSL code here, the only way we can really fail in
      the non-engine case is if malloc() fails.  But if malloc() is failing,
      then tor_malloc() calls should be tor_asserting -- the only way that an
      attacker could do an exploit here would be to figure out some way to
      make malloc() fail when openssl does it, but work whenever Tor does it.
      (Also ordinary malloc() doesn't fail on platforms like Linux that
      Observation 1b:
      Although engines are _allowed_ to fail in extra ways, I can't find much
      evidence online  that they actually _do_ fail in practice. More evidence
      would be nice, though.
      Observation 2:
      We don't call crypto_pk_generate*() all that often, and we don't do it
      in response to external inputs. The only way to get it to happen
      remotely would be by causing a hidden service to build new introduction
      Observation 3a:
      So, let's assume that both of the above observations are wrong, and the
      attacker can make us generate a crypto_pk_env_t with a dangling pointer
      in its 'key' field, and not immediately crash.
      This dangling pointer will point to what used to be an RSA structure,
      with the fields all set to NULL.  Actually using this RSA structure,
      before the memory is reused for anything else, will cause a crash.
      In nearly every function where we call crypto_pk_generate*(), we quickly
      use the RSA key pointer -- either to sign something, or to encode the
      key, or to free the key.  The only exception is when we generate an
      intro key in rend_consider_services_intro_points().  In that case, we
      don't actually use the key until the intro circuit is opened -- at which
      point we encode it, and use it to sign an introduction request.
      So in order to exploit this bug to do anything besides crash Tor, the
      attacker needs to make sure that by the time the introduction circuit
      completes, either:
        * the e, d, and n BNs look valid, and at least one of the other BNs is
          still NULL.
        * all 8 of the BNs must look valid.
      To look like a valid BN, *they* all need to have their 'top' index plus
      their 'd' pointer indicate an addressable region in memory.
      So actually getting useful data of of this, rather than a crash, is
      going to be pretty damn hard.  You'd have to force an introduction point
      to be created (or wait for one to be created), and force that particular
      crypto_pk_generate*() to fail, and then arrange for the memory that the
      RSA points to to in turn point to 3...8 valid BNs, all by the time the
      introduction circuit completes.
      Naturally, the signature won't check as valid [*], so the intro point
      will reject the ESTABLISH_INTRO cell.  So you need to _be_ the
      introduction point, or you don't actually see this information.
      [*] Okay, so if you could somehow make the 'rsa' pointer point to a
      different valid RSA key, then you'd get a valid signature of an
      ESTABLISH_INTRO cell using a key that was supposed to be used for
      something else ... but nothing else looks like that, so you can't use
      that signature elsewhere.
      Observation 3b:
      Your best bet as an attacker would be to make the dangling RSA pointer
      actually contain a fake method, with a fake RSA_private_encrypt
      function that actually pointed to code you wanted to execute.  You'd
      still need to transit 3 or 4 pointers deep though in order to make that
      By 1, you probably can't trigger this without Tor crashing from OOM.
      By 2, you probably can't trigger this reliably.
      By 3, even if I'm wrong about 1 and 2, you have to jump through a pretty
      big array of hoops in order to get any kind of data leak or code
      So I'm calling it a bug, but not a security hole. Still worth
    • Nick Mathewson's avatar
    • Nick Mathewson's avatar
      Fix a pointer arithmetic bug in memarea_alloc() · be2d37ad
      Nick Mathewson authored
      Fortunately, the arithmetic cannot actually overflow, so long as we
      *always* check for the size of potentially hostile input before
      copying it.  I think we do, though.  We do check each line against
      MAX_LINE_LENGTH, and each object name or object against
      MAX_UNPARSED_OBJECT_SIZE, both of which are 128k.  So to get this
      overflow, we need to have our memarea allocated way way too high up
      in RAM, which most allocators won't actually do.
      Bugfix on, where memarea was introduced.
      Found by Guido Vranken.
    • Nick Mathewson's avatar
      Use calloc, not malloc(a*b), in ed25519 batch signature check fn · 0ef36626
      Nick Mathewson authored
      [Not a triggerable bug unless somebody is going to go checking
      millions+ of signatures in a single go.]
  6. 24 May, 2016 1 commit
  7. 23 May, 2016 6 commits
  8. 20 May, 2016 3 commits