Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#2800 closed defect (fixed)

Resolve or postpone all XXX022/XXX021 items

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We should fix everything tagged XXX022 or XXX021 in the code, or triage it and decide that it can wait, before we release 0.2.2.x-rc.

I've got a branch in my public repository, "xxx_fixups", that tries to start doing most of it. There are 5 items remaining after that branch, one of which is #2799 .

Child Tickets

Change History (14)

comment:1 Changed 8 years ago by nickm

Status: newneeds_review

comment:2 Changed 8 years ago by Sebastian

Added a branch xxx_fixups to my repo

Fixed the comment a little for 41380fa3. Looks good otherwise.

c4bd06735: I'd probably just remove the lines. But keeping them seems fine too

05887f10ff: for the reasons.c change, we might want to do this still? 0.2.1.30 is out, and nobody should be running a versoin earlier than 0.2.1.27 anymore due to security concerns. The change in rephist.c seems important, but I think we don't need to block 0.2.2.x for that: Authorities are easy to upgrade and they all run alpha or more recent currently, so they could also run the 0.2.3 release containing the fix

dddd333a: I have a fixup commit in my branch

f3b89c114 and 43273427: about the stream_id exhaustion, maybe this XXX can also be turned into an attack where you manage to open a few 10k streams and see that a particular circuit dies/doesn't get new streams. Not sure how realistic that is, tho.

comment:3 Changed 8 years ago by Sebastian

what I haven't fully done is going through the unmarked XXX to see if any of them should be done in 022

comment:4 Changed 8 years ago by nickm

Merged your xxx_fixups branch onto mine, and made the reasons.c change you suggested.

re the stream_id exhaustion thing: I'm not sure how the attack would work exactly; can you explain more?

comment:5 Changed 8 years ago by Sebastian

Ah, I was confused about the attack. If the attacker has local access to Tor or can circumvent it, then it doesn't need to wait for a stream to close. Sorry for the noise here

comment:6 Changed 8 years ago by nickm

np; caution is a fine thing.

comment:7 Changed 8 years ago by arma

   /* Split entry guards into those on the list and those not. */
 
-  /* XXXX022 Now that we allow countries and IP ranges in EntryNodes, this is
+  /* XXXX023 Now that we allow countries and IP ranges in EntryNodes, this is
    *  potentially an enormous list. For now, we disable such values for
    *  EntryNodes in options_validate(); really, this wants a better solution.

This doesn't actually want to be an XXXX023 -- I already resolved it in master. (If you change it here you will find a conflict when you try to merge.)

Speaking of which, you might try merging this patch into master to see if there's anything else that conflicts.

comment:8 Changed 8 years ago by arma

-        expired = ftime_definitely_after(now, cert->expires);
+        expired = now > cert->expires;

...

-      if (!ftime_definitely_after(now, cert->expires)) {
+      if (now < cert->expires) {

This replacement doesn't produce exactly the same functionality. I'm not sure if we care.

For example, might the second one want to be "<="?

comment:9 Changed 8 years ago by arma

The rest of it looks ok, other than the fact that it doesn't look like it'll merge cleanly into master, so we'll have to do that part carefully.

comment:10 in reply to:  7 Changed 8 years ago by nickm

Replying to arma:

This doesn't actually want to be an XXXX023 -- I already resolved it in master. (If you change it here you will find a conflict when you try to merge.)

I'm going to leave it as XXX023, and resolve the conflict in favor of master when I merge. The alternative is to leave it as XXX022, which would be wrong, since we *don't* intend to fix it in 0.2.2.

Replying to arma:

This replacement doesn't produce exactly the same functionality. I'm not sure if we care.

I am pretty sure that we don't care. To get the original behavior, we'd actually want to add in 60 seconds of allowable delay between certificate expiry and deciding that it's actually expired. But I don't believe that really buys us anything.

comment:11 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged. We'll want to have a close look at the final merge commit (67d88a7d6021e95a2d423a9f26811accd1da39b6) to make sure I didn't wreck anything.

comment:12 Changed 8 years ago by Sebastian

I checked the diff here, it looks sane to me.

comment:13 Changed 7 years ago by nickm

Keywords: tor-client added

comment:14 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.