Project

General

Profile

Bug #11508

Bug #10288: Fix newly identified issues to make our test suite more robust and faster

Firewall leak detector makes bad assumptions about PacketFu parsing

Added by anonym over 2 years ago. Updated over 1 year ago.

Status:
Resolved
Priority:
Elevated
Assignee:
-
Category:
Test suite
Target version:
Start date:
06/04/2016
Due date:
% Done:

100%

QA Check:
Pass
Feature Branch:
Type of work:
Code
Blueprint:
Starter:
Affected tool:

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 assert@s 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.

Associated revisions

Revision 177243ae (diff)
Added by anonym over 1 year ago

Add debugging info for when PacketFu misbehaves.

Refs: #11508

Revision c29f5844
Added by intrigeri over 1 year ago

Merge remote-tracking branch 'origin/test/11508-possible-packetfu-bug' into testing (refs: #11508).

Revision 97f1eee4 (diff)
Added by anonym over 1 year ago

Test suite: fixup on debugging info logging for PacketFu.

We still get "undefined method `ip_saddr' for nil:NilClass" because
another instance of `ip_packet.ip_saddr` was forgotten and not dealt
with.

Refs: #11508

Revision fdd50f83 (diff)
Added by anonym over 1 year ago

Test suite: refine debug info collection of #11508.

I have seen yet another:

NoMethodError: undefined method `ip_saddr' for nil:NilClass

So we now know that `ip_packet` is not `nil`, it just doesn't have the
`ip_saddr` method (and probably not the `ip_daddr` one either) some
how (feels strange for an IP packet to not have a source or
destination address...).

Refs: #11508

Revision 112b34da (diff)
Added by anonym over 1 year ago

Test suite: try possible fix for #11508.

Yup, it seems that all along I've just missed that we could have
IPv6Packet:s in `ip_packet`, and their source is accessed by
`.ipv6_saddr`, not `ip_saddr` (that's for IPv4Packet). So, let's just
try and see which one of the two each `ip_packet` has, because one of
them must be there!

Also, given that UDPPacket can be either IPv4 or IPv6 it seems safest
to try to parse each packet as IPv6Packet first -- that way we keep
looking at transport layer protocols for IPv4 only, and treat
everything IPv6 as the same, which makes sense, since we should block
all IPv6, so everything should be treated the same at all times.

Refs: #11508

History

#1 Updated by intrigeri about 2 years ago

  • Parent task set to #10288

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 about 2 years ago

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

#3 Updated by anonym about 2 years ago

  • 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 almost 2 years ago

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

#6 Updated by anonym almost 2 years ago

  • Target version changed from Tails_2.9.1 to Tails 2.10

#7 Updated by intrigeri almost 2 years ago

  • Priority changed from Normal to Elevated

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

#8 Updated by anonym over 1 year ago

  • 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 over 1 year ago

  • 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 over 1 year ago

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

#11 Updated by anonym over 1 year ago

  • % Done changed from 30 to 40

I just pushed 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 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 over 1 year ago

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

#13 Updated by intrigeri over 1 year ago

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

#14 Updated by anonym over 1 year ago

  • 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 over 1 year ago

Congrats!

#16 Updated by intrigeri over 1 year ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF