-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Basic SMTP/IMAP probe: look for bad TLS and StartTLS downgrade attacks #989
base: master
Are you sure you want to change the base?
Conversation
IMAP outputOnly direct TLS output is presented here as StartTLS code is currently broken (see TODO in PR description). Example report:
Example output:
Example of TLS blocking with patched jafar:
Example report:
Example output:
|
SMTP outputExample report:
Example output:
Example of TLS blocking with #987:
Example report:
Example output:
|
|
||
port := "" | ||
|
||
if parsed.Port() == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps instead of setting a default port, we should throw an error due to a malformed input.
Since we generally use the input column for searching and filtering through measurements, a measurement run with as input imap://gmail.com
will not be grouped together with a measurement with input imap://gmail.com:143
.
ServerName: config.host, | ||
} | ||
|
||
runner := &tcprunner.TCPRunner{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should eventually put something like this into the standard measurement library. I think putting it as an experiment
package is sub-optimal.
@bassosimone where do you think something like this could go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should eventually put something like this into the standard measurement library. I think putting it as an experiment package is sub-optimal.
@bassosimone where do you think something like this could go?
Based on a quick skim through the package implementation itself, this code seems to me an abstraction built on top of measurexlite
. It seems to support well the use case of this experiment and may support alike experiments, so I think it's fine to give it a toplevel-internal directory name such as ./internal/tcprunner
.
BTW, the fact that @ooninoob felt the need to write a support package is very precious information. Our previous assumption was that people were going to write experiments using measurexlite
primitives. However, it seems this level of abstraction is too low to really be satisfactory. Because we also determined the declarative API of urlgetter
is also not flexible enough, it is clear to me there is need to invest some extra time to provide a more comprehensive abstraction on top of measurexlite
to support several use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be useful if i wrote about my personal feelings as a newcomer regarding the internal APIs? I could open a new ticket about this. Please note that i'm still not confident about what makes a good data structure for the test keys when it comes to testing multiple things (several hostnames and/or IP/port combos, as in these probes and the DHT/Bittorrent probes) and that consideration very much affects how an internal API should feel like.
I don't know what is a good answer and i for sure can't come up with specific guidelines, but it feels to me like there's different ways to approach the testkeys organization from a high-level perspective:
- global testkeys for everything (linear queries/tcp results/etc)
- global testkeys to store one invididual testkeys for each hostname (including DNS results)
- global testkeys (for DNS results) storing one individual testkeys for each IP resolved
- global testkeys storing one individual testkeys for each hostname, each storing one sub-individual testkeys for each resolved IP
It would sure be possible to have explicit guidelines about this, but just for starting to develop things we could have high-level APIs for several of those modes of operation. I think exposing a failed_step/failed_run as part of those high-level APIs would also bring value to help produce better test results (and better debugging abilities) without having each probe reinvent the wheel.
That is my personal opinion that probes should focus on their probing logic (networking/protocol stuff) and not have to know/care about ooni internals and how data is magically collected and presented. This would for sure lower the barrier for contribution for new probes, although i would understand if the ooni project willingly prefers to keep the barrier high and the implementation details explicit. I just felt from talking to @hellais and from reading your comment that it may not be a conscious design decision to keep me (and other future contributors) buried under ooni internal abstractions and that all this error-prone boilerplate may have been accidental ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ooninoob thank you for your very detailed reply!
Some comments inline:
different ways to approach the testkeys organization from a high-level perspective:
- global testkeys for everything (linear queries/tcp results/etc)
- global testkeys to store one invididual testkeys for each hostname (including DNS results)
- global testkeys (for DNS results) storing one individual testkeys for each IP resolved
- global testkeys storing one individual testkeys for each hostname, each storing one sub-individual testkeys for each resolved IP
I am not sure sure about what you mean here. Are you complaining about the data format being inconsistent across experiments or are you referring to the APIs available for writing experiments?
probes should focus on their probing logic (networking/protocol stuff) and not have to know/care about ooni internals and how data is magically collected and presented
I agree with the need to allow a programmer to focus on the network experiment logic. I find it to insightful to see you would like our documentation to spend less time about how data is collected and presented. I see why the current documentation focuses on that and why you rightfully say you should be (too much) concerned with that.
lower the barrier for contribution for new probes, although i would understand if the ooni project willingly prefers to keep the barrier high and the implementation details explicit
We previously had a high level API, which we now have deprecated. The reason why we deprecated it is that it was and is a burden to maintain in light of having a number of experiments to maintain. Each experiment has some set of semi-conflicting requirements, which shapes and puts pressure on such API.
You ended up using directly the internal primitives (aka "step-by-step") that we determined are "better", where better is defined in terms of the kind of experiments (and follow-up experiments) we can write. We were not sure whether there was a need to add another API on top of that or not. Your experience tells us that we definitely need to offer some kind of API for people to write experiments that is more abstract.
it may not be a conscious design decision to keep me (and other future contributors) buried under ooni internal abstractions and that all this error-prone boilerplate may have been accidental
Yeah, it's not a conscious decision. I suppose it's more of a learning process where, on the one end, we need to adapt our tools for measuring censorship and detecting which low-level step failed and possibly try to run some follow-up test. On the other end, we also need to provide contributors with easy to use abstractions. There's definitely a time lag in there and it's also true that it's difficult to know the direction without feedback, so thank you for your feedback!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Oh, @ooninoob, if you have time to chat about this, perhaps we can have a chat on Slack and then distill the result of the chat in some issue or design document to improve the contributing experience.)
TCPConnect []*model.ArchivalTCPConnectResult `json:"tcp_connect"` | ||
TLSHandshake *model.ArchivalTLSOrQUICHandshakeResult `json:"tls_handshakes"` | ||
Failure string `json:"failure"` | ||
FailureStep string `json:"failed_step"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FailureStep string `json:"failed_step"` | |
FailedStep string `json:"failed_step"` |
// FailedStep TCPSessionModel | ||
func (itk *IndividualTestKeys) FailedStep(failure string, step string) { | ||
itk.Failure = failure | ||
itk.FailureStep = step |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
itk.FailureStep = step | |
itk.FailedStep = step |
TCPConnect []*model.ArchivalTCPConnectResult `json:"tcp_connect"` | ||
TLSHandshake *model.ArchivalTLSOrQUICHandshakeResult `json:"tls_handshakes"` | ||
Failure string `json:"failure"` | ||
FailureStep string `json:"failed_step"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FailureStep string `json:"failed_step"` | |
FailedStep string `json:"failed_step"` |
// FailedStep TCPSessionModel | ||
func (itk *IndividualTestKeys) FailedStep(failure string, step string) { | ||
itk.Failure = failure | ||
itk.FailureStep = step |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
itk.FailureStep = step | |
itk.FailedStep = step |
func (itk *IndividualTestKeys) FailedStep(failure string, step string) { | ||
itk.Failure = failure | ||
itk.FailureStep = step | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you really need setters for the individualTestKeys
attributes. You can just set them directly in the body of the experiment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I introduced this helper method to be sure i wouldn't miss one of the two keys when reporting an error. Is this unreasonable? Also, the field is named FailureStep precisely because this method FailedStep creates a naming conflict. I could rename it of course if you think it's wise to keep this helper method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding to what @hellais said:
The use case where you need setters I can think of is the one in which you're running operations in parallel and therefore want to protect the test keys with a mutex. In such a case, I suggest this pattern:
type TestKeys struct {
Foo string `json:"foo"`
mu *sync.Mutex // not exported because lowercase
}
func (tk *TestKeys) setFoo(value string) {
defer tk.mu.Unlock()
tk.mu.Lock()
tk.Foo = value
}
For cases in which you're running operations in parallel, I would recommend setting keys directly.
return nil | ||
} | ||
|
||
func testIMAP(s *tcprunner.TCPSession, noop uint8) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very bike-sheddy comment, but maybe these functions should not be named with the test
prefix to avoid confusion with the Test
functions inside of a unit/integration test.
internal/tcprunner/tcprunner.go
Outdated
Port string | ||
TLS bool | ||
RawConn *net.Conn | ||
TLSConn *net.Conn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed out of band the issue about the deadlock in the startTLS method. While it's still unclear to me exactly what the root cause is, one problem in the current code is that these RawConn
and TLSConn
should not be pointers (net.Conn
is an interface), but an actual copy to the relevant connection.
net.Conn
makes some guarantees about it being possible to call methods on them also concurrently, which means it has in it some locking to deal with concurrency and is probably what is leading to the deadlock.
I haven't fully understood where exactly this is happening, but I suspect it has something to do with the fact that there is this proxy object managing the two connections.
My suggestion would be to refactor the code to get rid of this proxy object and use the net.Conn
objects directly. You don't even need to handle the special case for startTLS, as you can conditionally perform startTLS on the plaintext connection and swap it out for the original net.Conn
once you have an encrypted tunnel.
That should simplify the code and hopefully make it more clear where the deadlock is happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So i just pushed a new commit with this suggestion. Unfortunately the code is still deadlocking. strace log:
[ 5.571522] Initializing SMTP client
epoll_pwait(3, [], 128, 0, NULL, 0) = 0
write(5, "\0", 1) = 1
futex(0x2482168, FUTEX_WAIT_PRIVATE, 0, NULL) = 0
futex(0x2482168, FUTEX_WAIT_PRIVATE, 0, NULL) = 0
epoll_pwait(3, [], 128, 0, NULL, 0) = 0
futex(0x2482168, FUTEX_WAIT_PRIVATE, 0, NULL) = 0
epoll_pwait(3, [], 128, 0, NULL, 0) = 0
nanosleep({tv_sec=0, tv_nsec=3000}, NULL) = 0
write(5, "\0", 1) = 1
futex(0x2482168, FUTEX_WAIT_PRIVATE, 0, NULL) = 0
epoll_pwait(3, [], 128, 0, NULL, 0) = 0
futex(0xc000088148, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x2482168, FUTEX_WAIT_PRIVATE, 0, NULL) = 0
futex(0x2482168, FUTEX_WAIT_PRIVATE, 0, NULL) = 0
futex(0x2482168, FUTEX_WAIT_PRIVATE, 0, NULL
I have reviewed the PR and done some light testing of it. I think once we resolve the deadlocking issue this is pretty much feature complete WRT being a functioning test. The other remaining bits that we will have to address before we can merge it into master are:
I think that WRT the hackathon, the challenge is to be considered complete, as it has for sure met what I think can be expected from somebody to do in a few days of work. |
Checklist
Description
As part of the InternetBorders hackathon, i am submitting a work-in-progress pull request to add SMTP/IMAP TLS blocking and StartTLS downgrade attacks detection support in miniooni. I was mentored by @hellais during the hackathon, who introduced me to the many flavors of ooni APIs.
I am aware that the contribution guidelines favor small patches, however these two small modules consist of mostly the same code, to the point that i introduced a new helper library for plaintext TCP + StartTLS. Please let me know if i should submit three pull requests instead.
There is still a moderate amount of work to do before merging, and to be honest i'm a little ashamed of the state of this pull request. I would never have submitted yet if not for the hackathon deadline. I am however ready to dedicate at least 3 more days of my time full-time in order to get this pull request merged, given sufficient/guidance.
The biggest blocker to merging that i have no idea how to fix is a futex deadlock in the StartTLS code that i introduced at some point during refactoring. See TODO for more information.
Input
The smtp probe expects a
smtp://hostname[:port]
input for StartTLS orsmtps://hostname[:port]
for direct TLS. If no port is provided, ports 587 for StartTLS and 465 for direct TLS are implied.The imap probe expects a
imap://hostname[:port]
input for StartTLS orimaps://hostname[:port]
for direct TLS. If no port is provided, ports 143 for StartTLS and 993 for direct TLS are implied.Output
Github says PR body is too long so i will post sample output/report and test results in comments
IMAP has the following data in the report's test keys:
Where an individual test run contains:
The IMAP failed_step can be:
SMTP has the following data in the report's test keys:
Where an individual test run contains:
The SMTP failed_step can be:
TODO
The only hard blocker that may require some active help from a more experience gopher is the broken StartTLS. It seemed to be working fine previously (will submit previous code in a further comment) until i refactored everything. Right now it just deadlocks until timeout 5 minutes later (why 5 minutes and not 1 minute timeout set in run context?). Running under strace reveals some issue related to FUTEX.
I suspect it may have to do with introducing a reader to check for StartTLS ready message, but i have exactly zero experience with golang (or golang networked programming) so i don't have any idea where to start investigating. The issue may be obvious to a more experienced gopher as the code is not so convoluted.