Bug 140409

Comment 1 Edwin Groothuis 2009-11-09 10:00:23 UTC

Maintainer of security/tor-devel,

Please note that PR ports/140409 has just been submitted.

If it contains a patch for an upgrade, an enhancement or a bug fix
you agree on, reply to this email stating that you approve the patch
and a committer will take care of it.

The full text of the PR can be found at:
    http://www.freebsd.org/cgi/query-pr.cgi?pr=ports/140409

-- 
Edwin Groothuis via the GNATS Auto Assign Tool
edwin@FreeBSD.org

Comment 2 Edwin Groothuis 2009-11-09 10:00:25 UTC

State Changed
From-To: open->feedback

Awaiting maintainers feedback (via the GNATS Auto Assign Tool)

Comment 3 Martin Wilke 2009-11-09 11:14:56 UTC

Responsible Changed
From-To: freebsd-ports-bugs->miwi

I'll take it.

Hi!




http://archives.seul.org/or/cvs/Nov-2009/msg00030.html



Patch FreeBSD port - ports/security/tor-devel:


-----------------------

--- src/common/tortls.c
+++ src/common/tortls.c
@@ -154,6 +154,7 @@
                                          const char *cname,
                                          const char *cname_sign,
                                          unsigned int lifetime);
+static void tor_tls_unblock_renegotiation(tor_tls_t *tls);

  /** Global tls context. We keep it here because nobody else needs to
   * touch it. */
@@ -927,6 +928,36 @@
  #endif
  }

+/** If this version of openssl requires it, turn on renegotiation on
+ * <b>tls</b>.  (Our protocol never requires this for security, but 
it's nice
+ * to use belt-and-suspenders here.) 

+ */ 

+static void
+tor_tls_unblock_renegotiation(tor_tls_t *tls)
+{
+#ifdef SSL3_FLAGS_ALLOW_UNSAFE_LEGACY_RENEGOTIATION
+  /* Yes, we know what we are doing here.  No, we do not treat a 
renegotiation
+   * as authenticating any earlier-received data. */
+  tls->ssl->s3->flags |= SSL3_FLAGS_ALLOW_UNSAFE_LEGACY_RENEGOTIATION;
+#else
+  (void)tls;
+#endif
+}
+
+/** If this version of openssl supports it, turn off renegotiation on
+ * <b>tls</b>.  (Our protocol never requires this for security, but 
it's nice
+ * to use belt-and-suspenders here.)
+ */
+void
+tor_tls_block_renegotiation(tor_tls_t *tls)
+{
+#ifdef SSL3_FLAGS_ALLOW_UNSAFE_LEGACY_RENEGOTIATION
+  tls->ssl->s3->flags &= ~SSL3_FLAGS_ALLOW_UNSAFE_LEGACY_RENEGOTIATION;
+#else
+  (void)tls;
+#endif
+}
+
  /** Return whether this tls initiated the connect (client) or
   * received it (server). */
  int
@@ -1058,6 +1089,9 @@
    if (oldstate != tls->ssl->state)
      log_debug(LD_HANDSHAKE, "After call, %p was in state %s",
                tls, ssl_state_to_string(tls->ssl->state));
+  /* We need to call this here and not earlier, since OpenSSL has a 
penchant
+   * for clearing its flags when you say accept or connect. */
+  tor_tls_unblock_renegotiation(tls);
    r = tor_tls_get_error(tls,r,0, "handshaking", LOG_INFO, LD_HANDSHAKE);
    if (ERR_peek_error() != 0) {
      tls_log_errors(tls, tls->isServer ? LOG_INFO : LOG_WARN, LD_HANDSHAKE,
--- src/common/tortls.h
+++ src/common/tortls.h
@@ -65,6 +65,7 @@ int tor_tls_read(tor_tls_t *tls, char *cp, size_t len);
  int tor_tls_write(tor_tls_t *tls, const char *cp, size_t n);
  int tor_tls_handshake(tor_tls_t *tls);
  int tor_tls_renegotiate(tor_tls_t *tls);
+void tor_tls_block_renegotiation(tor_tls_t *tls);
  int tor_tls_shutdown(tor_tls_t *tls);
  int tor_tls_get_pending_bytes(tor_tls_t *tls);
  size_t tor_tls_get_forced_write_size(tor_tls_t *tls);
--- src/or/connection_or.c
+++ src/or/connection_or.c
@@ -844,6 +844,7 @@ connection_or_tls_renegotiated_cb(tor_tls_t *tls, 
void *_conn)

    /* Don't invoke this again. */
    tor_tls_set_renegotiate_callback(tls, NULL, NULL);
+  tor_tls_block_renegotiation(tls);

    if (connection_tls_finish_handshake(conn) < 0) {
      /* XXXX_TLS double-check that it's ok to do this from inside read. */
@@ -1087,6 +1088,7 @@ connection_tls_finish_handshake(or_connection_t *conn)
        connection_or_init_conn_from_address(conn, &conn->_base.addr,
                                             conn->_base.port, 
digest_rcvd, 0);
      }
+    tor_tls_block_renegotiation(conn->tls);
      return connection_or_set_state_open(conn);
    } else {
      conn->_base.state = OR_CONN_STATE_OR_HANDSHAKING;

--------------------



-- 
  Best regards, Andrei V. Lavreniyuk.

I have no way to test this (and broke my damn foot this morning) but if 
it looks ok then approved.

NOTE: Is this even needed?  My assumption is the tor folk will fix this 
in the next release and not sure this is a security bug.

-Peter

On 11/9/09, Peter Thoenen <peter.thoenen@yahoo.com> wrote:
> I have no way to test this (and broke my damn foot this morning) but if
> it looks ok then approved.
>
> NOTE: Is this even needed?  My assumption is the tor folk will fix this
> in the next release and not sure this is a security bug.
>
> -Peter
>

Ouch.  I hope you're better soon.  Like I wrote in my earlier private
email, tor works safely with the openssl from the base system.  But if
you use the openssl from the base system, you have to use it for all
ports, not just tor (unless you make some local changes).  And other
ports may be vulnerable if they use the now unpatched openssl from the
base system.  So it's a security problem in that sense.  If you try to
make the other ports safe by switching to the new openssl from ports,
you break tor.  The tor developers adopted one of the solutions I
mentioned (and the one that seems to have been used in this PR) in the
latest sources, which will probably be used in the next release, but
for now it is broken. I'm willing to wait for the new release (I've
fixed it on my system :) ), but I don't know if everyone else is, so I
thought I'd let you know, and that is why I wrote my earlier message.

b.

Please consider instead the following patch, which updates the port to
0.2.2.6-alpha.  This includes ssl/tls changes, so that with this
update, tor-devel should work with both the base system and port
versions of openssl.  I also changed the regression-test target to a
test target, so that a failed timeout assertion in the util/threads
regression test on -CURRENT doesn't break builds on some tinderboxes
that run the regression-test target automatically.

As usual, gmail may automatically encode this patch in base-64, so try
to decode the patch if it appears garbled.

b.

Approved .. one day I will settle now and have normal internet so I can 
get back to using FBSD :)

-Peter

Comment 9 Martin Wilke 2009-11-21 23:02:26 UTC

State Changed
From-To: feedback->closed

Committed. Thanks!