Bug #11508

Firewall leak detector makes bad assumptions about PacketFu parsing

Added by anonym 2016-06-04 13:10:18 . Updated 2017-05-23 09:08:49 .

Status:
Resolved
Priority:
Elevated
Assignee:
Category:
Test suite
Target version:
Tails_3.0~rc1
Start date:
2016-06-04
Due date:
% Done:

100%

Feature Branch:
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:

Description

I’ve seen this:

calling as amnesia: host -T torproject.org 208.67.222.222
call returned: [0, "Using domain server:\nName: 208.67.222.222\nAddress: 208.67.222.222#53\nAliases: \n\ntorproject.org has address 86.59.30.40\ntorproject.org has address 38.229.72.16\ntorproject.org has address 82.195.75.101\ntorproject.org has address 138.201.14.197\ntorproject.org has IPv6 address 2001:41b8:202:deb:213:21ff:fe20:1426\ntorproject.org has IPv6 address 2001:858:2:2:aabb:0:563b:1e28\ntorproject.org has IPv6 address 2a01:4f8:172:1b46:0:abba:5:1\ntorproject.org has IPv6 address 2620:0:6b0:b:1a1a:0:26e5:4810\ntorproject.org mail is handled by 10 eugeni.torproject.org.\n", ""]
    When I do a TCP DNS lookup of "torproject.org"                                        # features/step_definitions/firewall_leaks.rb:19
    Then the firewall leak detector has detected leaks                                    # features/step_definitions/firewall_leaks.rb:1
      <Test::Unit::AssertionFailedError> exception expected but was
      <NoMethodError(<undefined method `ip_saddr' for nil:NilClass>)>. (Test::Unit::AssertionFailedError)
      ./features/step_definitions/firewall_leaks.rb:2:in `/^the firewall leak detector has detected leaks$/'
      features/tor_enforcement.feature:33:in `Then the firewall leak detector has detected leaks'


This implies that the various PacketFu .parse methods can return nil despite .can_parse? returning true. This feels like a bug in PacketFu.

Sadly the way this error happened prevented a .pcap artifact to be saved. Also:

Then(/^the firewall leak detector has detected leaks$/) do
  assert_raise(Test::Unit::AssertionFailedError) do
    step 'all Internet traffic has only flowed through Tor'
  end
end


That step calls assert_all_connections and given how we have the opposite expectation about the assertion (assert_raise) the artifact saving occurs in the opposite of what we want here.

Also: probably we’ll end up throwing in some asserts in the pcap_connections_helper (e.g. for the nil issue above) so we should probably introduce another exception to throw when assert_all_connections fails, and a subclass of Test::Unit::AssertionFailedError would be good enough to make it possible to distinguish.

Subtasks

History

#1 Updated by intrigeri 2016-07-28 06:20:40

Seen this today: https://jenkins.tails.boum.org/job/test_Tails_ISO_bugfix-10720-installer-freezes-on-jenkins/72/

Shall we mark all scenarios that have “the firewall leak detector has detected leaks” as fragile?

#2 Updated by anonym 2016-08-31 05:09:00

  • Target version changed from Tails_2.6 to Tails_2.7

#3 Updated by anonym 2016-09-01 16:13:26

  • Status changed from Confirmed to In Progress
  • % Done changed from 0 to 10
  • Feature Branch set to wip/11508-possible-packetfu-bug

Pushing a WIP branch: wip/11508-possible-packetfu-bug

#5 Updated by bertagaz 2016-11-17 17:38:35

  • Target version changed from Tails_2.7 to Tails_2.9.1

#6 Updated by anonym 2016-12-14 20:11:21

  • Target version changed from Tails_2.9.1 to Tails 2.10

#7 Updated by intrigeri 2016-12-20 10:55:29

  • Priority changed from Normal to Elevated

This happened 8 times on stable+devel in Oct+Nov => raising priority.

#8 Updated by anonym 2017-01-21 15:12:39

  • Assignee changed from anonym to intrigeri
  • % Done changed from 10 to 30
  • QA Check set to Ready for QA
  • Feature Branch changed from wip/11508-possible-packetfu-bug to test/11508-possible-packetfu-bug

For now the feature branch contains no fix, only better data gathering. Please merge’n’review into testing (and then into devel), but do not close this ticket (just move the Target version to 2.11 and reassign to me as Dev needed)!

#9 Updated by intrigeri 2017-01-22 11:41:37

  • Assignee changed from intrigeri to anonym
  • Target version changed from Tails 2.10 to Tails_2.11
  • QA Check changed from Ready for QA to Dev Needed

Done!

#10 Updated by anonym 2017-03-05 16:23:19

  • Target version changed from Tails_2.11 to Tails_2.12

#11 Updated by anonym 2017-03-19 02:00:43

  • % Done changed from 30 to 40

I just pushed commit:fdd50f83f44aa9f46f08ab8bbedf646b2efb3a06 (to stable -> devel -> feature/stretch). From the commit message:

I have seen yet another:

    NoMethodError: undefined method `ip_saddr' for nil:NilClass

So we now know that `ip_packet` is not `nil` [...]

I based that on the fact that the only instance of ip_saddr is

if ip_packet
  packet_info[:saddr] = ip_packet.ip_saddr
[...]


So, indeed, ip_packet cannot be nil. But if we look at the exception from above again we have: “undefined method `ip_saddr’ for nil:NilClass”. That instead seems to indicate that ip_packet is nil. WTF? Also, I just realized IPv6Packet has no ip_saddr, but a ipv6_saddr instead! Whoops! :S So a NoMethodError is indeed to be expected here, but it shouldn’t have said nil:NilClass, but [...]:IPv6Packet. However, after having carefully read the PacketFu sources, I now see that nil:NilClass is to be expected given the implementation, so that exception is actually raised from within the packetfu module. Urgh, well this has confused me quite a bit!

I also noticed that UDPPacket can be both IPv4 and IPv6, with either .ip_saddr or .ipv6_saddr. That combined with the above lead me to commit:112b34da0648706850d3f780dc620c8de444c33e which I have some hope will fix this ticket once and for all. Wooh! I’m pushing it to stable -> devel -> feature/stretch since we need wide exposure to get any confidence this bug indeed is fixed. I did successfully run the most relevant automated tests (tor_enforcement.feature in particular) before pushing, so I doubt this will make the situation any worse.

#12 Updated by anonym 2017-04-19 17:40:18

  • Target version changed from Tails_2.12 to Tails_3.0~rc1

#13 Updated by intrigeri 2017-05-16 16:31:32

  • QA Check changed from Dev Needed to Ready for QA
  • Feature Branch deleted (test/11508-possible-packetfu-bug)

#14 Updated by anonym 2017-05-17 11:38:02

  • Status changed from In Progress to Fix committed
  • Assignee deleted (anonym)
  • % Done changed from 40 to 100
  • QA Check changed from Ready for QA to Pass

I did this:

anonym@jenkins:~$ grep ip_saddr /var/lib/jenkins/jobs/test_Tails_ISO_*/builds/2017-03-*/archive/build-artifacts/debug.log | wc -l
43
anonym@jenkins:~$ grep ip_saddr /var/lib/jenkins/jobs/test_Tails_ISO_*/builds/2017-04-*/archive/build-artifacts/debug.log | wc -l
0

So:

  • the fix was pushed on March 20
  • in March there was 43 occurrences of this issue
  • in April none

=> I call this fixed!

#15 Updated by intrigeri 2017-05-17 12:13:56

Congrats!

#16 Updated by intrigeri 2017-05-23 09:08:49

  • Status changed from Fix committed to Resolved