Opened 8 years ago

Last modified 18 months ago

#4271 new defect

Perform some integrity checking in bw auth fetches

Reported by: mikeperry Owned by:
Priority: Medium Milestone:
Component: Core Tor/Torflow Version:
Severity: Normal Keywords:
Cc: karsten, aagbsn, mikeperry Actual Points:
Parent ID: #13630 Points:
Reviewer: Sponsor:

Description

We currently do not perform any certificate validation or length checking for the bw auths. We should do one or the other, or both.

Child Tickets

Change History (10)

comment:1 Changed 8 years ago by aagbsn

see this blog post that describes how to do certificate verification with urllib2:
http://thejosephturner.com/blog/2011/03/19/https-certificate-verification-in-python-with-urllib2/

and a work-in-progress:
https://gitweb.torproject.org/user/aagbsn/torflow.git/shortlog/refs/heads/4271-integrity-checking

note: it looks like the self-signed cert for 38.229.70.2 is not signed by a CA; this can be managed pretty easily with easy-rsa (bundled with openvpn).

comment:2 in reply to:  1 Changed 8 years ago by aagbsn

Replying to aagbsn:

see this blog post that describes how to do certificate verification with urllib2:
http://thejosephturner.com/blog/2011/03/19/https-certificate-verification-in-python-with-urllib2/

and a work-in-progress:
https://gitweb.torproject.org/user/aagbsn/torflow.git/shortlog/refs/heads/4271-integrity-checking

note: it looks like the self-signed cert for 38.229.70.2 is not signed by a CA; this can be managed pretty easily with easy-rsa (bundled with openvpn).

You can also just add the certificate to the ca_cert file ("bwauthority_certs"). Should we make the filename a configuration option in bwauthority.cfg? Should the certificate be in the repo? (I'd argue it's not much worse than the hardcoded urls we presently have, but we probably should have a better way to configure urls and certificates).

And someone should probably validate that the certificate I added is actually the right one.

Also, all we do here is make noise when SSL verification fails. Should we make a more significant effort to get attention?

comment:3 Changed 8 years ago by mikeperry

The repo is fine for storing the certificates. Though you'll note in mikeperry/pid_control I added the ability to randomly choose one url from a list of URLs. We need to support multiple URLs for that.

As for making noise.. Hrmm.. Let's do baby steps for that. Any incremental improvement on validation is good here, but we don't want to allow arbitrary SSL MITMs to break or otherwise delay the bw scan, and finding them is the exit authority's job.

Therefore, I think "log a WARN in bwauthority_child, but fallback to unverified download" is the best option.

We should make another ticket for figuring out how to make aggregate.py properly report all bw.log WARNs and ERRORs from all four child scanners.

comment:4 Changed 8 years ago by mikeperry

Created #4445 for the reporting.

comment:5 in reply to:  3 Changed 7 years ago by aagbsn

Replying to mikeperry:

The repo is fine for storing the certificates. Though you'll note in mikeperry/pid_control I added the ability to randomly choose one url from a list of URLs. We need to support multiple URLs for that.

We just need to concatenate all signing certs/signing CA's into the same cacert file.

As for making noise.. Hrmm.. Let's do baby steps for that. Any incremental improvement on validation is good here, but we don't want to allow arbitrary SSL MITMs to break or otherwise delay the bw scan, and finding them is the exit authority's job.

If the certificate does not validate (SSL MITM), the scan of that host is aborted. I suspect that having some hosts effectively unscanned is undesirable.

Therefore, I think "log a WARN in bwauthority_child, but fallback to unverified download" is the best option.

I agree.

comment:6 Changed 7 years ago by mikeperry

Owner: changed from mikeperry to aagbsn
Priority: normalmajor
Status: newassigned

comment:7 Changed 19 months ago by teor

Parent ID: #13630
Severity: Blocker

This is a feature that belongs in the new bwauth replacement project, see #13630.

comment:8 Changed 19 months ago by teor

Priority: HighMedium
Severity: BlockerNormal

Priorities and Severities in torflow are meaningless, setting them all to Medium/Normal.

comment:9 Changed 19 months ago by teor

Owner: aagbsn deleted

aagbsn was the default owner, unassigning

comment:10 Changed 18 months ago by teor

Status: assignednew

Mark all tickets that are assigned to nobody as "new".

Note: See TracTickets for help on using tickets.