Project

General

Profile

Feature #8514

Feature #6183: Use multiple PGP/MIME attachments for text snippets and OpenPGP public key included in Whisperback reports

Replace WhisperBack.mail_appended_info with a dictionary

Added by alant over 4 years ago. Updated about 1 year ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
-
Target version:
Start date:
01/02/2015
Due date:
% Done:

100%

Estimated time:
1.00 h
Feature Branch:
feature/8514-7180-whisperback-remove-right-pane-and-refactor,saschamarkus/whisperback:feature/8514-Replace-mail_appended_info-with-a-dictionary
Type of work:
Code
Blueprint:
Starter:
Affected tool:
WhisperBack

Description

Currently we set WhisperBack.mail_appended_info to a big string containg all debugging inforamtion. We should replace this by a dictionnary of {filename:content, filename:content, ...} so that we can attach each in a separate MIME attachement.

0001-feature-8514-Replace-WhisperBack.mail_appended_info-.patch View - tails repo patch (8.79 KB) sascha.markus@gmail.com, 05/30/2018 02:15 PM

0002-feature-8514-Replace-WhisperBack.mail_appended_info-.patch View - whisperback repo patch (1.98 KB) sascha.markus@gmail.com, 05/30/2018 02:15 PM

debug.json (1.25 KB) sascha.markus@gmail.com, 06/11/2018 09:17 PM


Related issues

Related to Tails - Feature #15793: Improve ownership test in tails-debugging-info Confirmed 08/15/2018
Blocks Tails - Feature #8722: Create MIME/Multipart message with attachments in WhisperBack In Progress 01/18/2015
Blocked by Tails - Bug #15472: Rebase our Tor Browser AppArmor policy on top of torbrowser-launcher 0.2.9-2's Resolved 03/28/2018
Blocks Tails - Feature #13241: Core work: Test suite maintenance Rejected 06/29/2017

Associated revisions

Revision c9f3bec3
Added by intrigeri over 1 year ago

Merge remote-tracking branch 'saschamarkus/feature/8514-Replace-WhisperBack.mail_appended_info-with-a-dictionary' into feature/8514-7180-whisperback-remove-right-pane-and-refactor (refs: #8514, #7180)

Revision 115dfb4f (diff)
Added by intrigeri over 1 year ago

Enable the feature-8514-7180-whisperback-remove-right-pane-and-refactor APT overlay (refs: #8514).

Revision 85cea6e6 (diff)
Added by intrigeri about 1 year ago

Test suite: adjust to the fact the WhisperBack debugging info is now stored in a machine-readable file (refs: #8514)

… so we don't have to "parse" a script shell anymore :)

Revision 37213b93 (diff)
Added by intrigeri about 1 year ago

Test suite: remove no-op test (refs: #8514)

Revision d24273ff
Added by intrigeri about 1 year ago

Merge branch 'feature/8514-7180-whisperback-remove-right-pane-and-refactor' into devel (Fix-committed: #8514, #7180)

History

#1 Updated by alant over 4 years ago

#2 Updated by alant over 4 years ago

  • Subject changed from Replace @WhisperBack.mail_appended_info@ by a dictionnary to Replace WhisperBack.mail_appended_info by a dictionnary

#3 Updated by intrigeri over 4 years ago

  • Subject changed from Replace WhisperBack.mail_appended_info by a dictionnary to Replace WhisperBack.mail_appended_info with a dictionnary

#4 Updated by intrigeri over 4 years ago

  • Subject changed from Replace WhisperBack.mail_appended_info with a dictionnary to Replace WhisperBack.mail_appended_info with a dictionary
  • Status changed from New to Confirmed

#5 Updated by alant over 4 years ago

  • Blocks deleted (Feature #8513: Implement PGP/MIME in WhisperBack)

#6 Updated by alant over 4 years ago

  • Blocks Feature #8722: Create MIME/Multipart message with attachments in WhisperBack added

#7 Updated by sascha.markus@gmail.com over 1 year ago

Hi alant
Here is a draft version to have dicts instead of a big string.
In config/chroot_local-includes/usr/local/sbin/tails-debugging-info the debug info is created as a dict and printed out serialized as json.
https://github.com/saschamarkus/tails/commit/1a52d1a9b063ca47fe999249178b6d4438a66356

This json is deserialized in the client code in whisperBack/whisperback.py
https://github.com/saschamarkus/whisperback/commit/240414182f6329c6f43bd5c17d100248c26ed873

This draft doesn't contain the output of /bin/journalctl because the deserialization failed.
I'll check how to solve this if you decide that this approach is OK.

#8 Updated by intrigeri over 1 year ago

  • Assignee changed from alant to intrigeri

(I doubt Alan will have time to look into this any time soon so I will.)

#9 Updated by intrigeri over 1 year ago

  • Feature Branch changed from https://github.com/saschamarkus/whisperback/tree/feature/8514-Replace-WhisperBack.mail_appended_info-with-a-dictionary to saschamarkus/whisperback:feature/8514-Replace-WhisperBack.mail_appended_info-with-a-dictionary

#10 Updated by intrigeri over 1 year ago

  • Status changed from Confirmed to In Progress
  • Assignee changed from intrigeri to sascha.markus@gmail.com
  • % Done changed from 0 to 10
  • QA Check changed from Info Needed to Dev Needed

Your branch contains quite some unrelated changes such as your #7180 work and your #6432 patch (that was applied in master with a different commit ID). Can you please rebase it on current master and ensure it contains only changes related to this ticket? If for some reason this work has to be based on top of #7180, fine: then let's mark this ticket as blocked by #7180 so that we review and merge them in the right order :)

#11 Updated by sascha.markus@gmail.com over 1 year ago

intrigeri wrote:

Your branch contains quite some unrelated changes such as your #7180 work and your #6432 patch (that was applied in master with a different commit ID). Can you please rebase it on current master and ensure it contains only changes related to this ticket? If for some reason this work has to be based on top of #7180, fine: then let's mark this ticket as blocked by #7180 so that we review and merge them in the right order :)

I have to admit that I failed with rebasing my change. So I just created a new branch on top of the latest master:
https://github.com/saschamarkus/whisperback/compare/feature/8514-Replace-mail_appended_info-with-a-dictionary

Now I'll go on and will check why deserializing the output of journalctl fails.

#12 Updated by sascha.markus@gmail.com over 1 year ago

  • Assignee changed from sascha.markus@gmail.com to intrigeri

Sanitizing the hardware info broke the json. Solved now.

tails-debugging-info in the tails repo now "returns" a list of dicts instead a dict of dicts to keep the order of the infos.
https://github.com/saschamarkus/tails/tree/feature/8514-Replace-WhisperBack.mail_appended_info-with-a-dictionary

The client code was updated to match this change.
https://github.com/saschamarkus/whisperback/tree/feature/8514-Replace-mail_appended_info-with-a-dictionary

#13 Updated by intrigeri over 1 year ago

  • QA Check changed from Dev Needed to Ready for QA
  • Feature Branch changed from saschamarkus/whisperback:feature/8514-Replace-WhisperBack.mail_appended_info-with-a-dictionary to saschamarkus/whisperback:feature/8514-Replace-mail_appended_info-with-a-dictionary,saschamarkus/tails:feature/8514-Replace-WhisperBack.mail_appended_info-with-a-dictionary

#14 Updated by intrigeri over 1 year ago

  • Assignee changed from intrigeri to sascha.markus@gmail.com
  • Target version set to Tails_3.9
  • % Done changed from 10 to 20
  • QA Check changed from Ready for QA to Dev Needed

Great! I'm glad we're making progress towards #6183, which will incidentally fix nasty problems such as #15470 (that makes our help desk's and users' life more painful than it should be). Once we're done here it would be awesome if you tackled #8722, ideally by the end of July.

Code review (not tested the code yet):

  • Any reason why https://github.com/saschamarkus/tails/commit/1a52d1a9b063ca47fe999249178b6d4438a66356 removes the automated tests embedded in the script?
  • In "Return the command and it's result as dict", maybe s/result/standard output/?
  • At least of one your commits introduces trailing whitespace and very long lines. Please fix this. (I recommend using linting helpers such as pep8, pylint and friends :)
  • Here we're changing the config API (mail_appended_info must now return valid JSON) so please update doc/config.py.sample and the docstring in config/chroot_local-includes/etc/whisperback/config.py.

Other than this, looks good! Let's aim to have this merged in time for 3.9 :)

#15 Updated by sascha.markus@gmail.com over 1 year ago

  • Assignee changed from sascha.markus@gmail.com to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

The branches in both repos are updated.
The files are checked with pep8. Some lines still have >79 chars but aren't very long anymore.
I moved the documentation for mail_prepended_info and mail_append_info from the comments to the docstring part.
The tests for main and this_command are back. I had to add the empty print() again to make them pass because I didn't see another way to test for "anything".

#16 Updated by intrigeri over 1 year ago

  • Assignee changed from intrigeri to sascha.markus@gmail.com
  • % Done changed from 20 to 30
  • QA Check changed from Ready for QA to Dev Needed

Almost there! Another code review (still not tested):

  • I think it's standard input is wrong: you mean "standard output" instead, right?
  • OrderedDict is not used anymore, is it? then let's not import it :)
  • config.py.sample: please keep docstrings generic (you've added Tails-specific requirements in there while the idea is that whoever uses WhisperBack can override these functions to prepend/append/etc. whatever info they want, which might not be related to Tails)

Notes for the future regarding your Git history:

  • atomic commits would be so muuuuch nicer (to review, to bisect, etc.)
  • please don't include in a commit message anything that is not about that commit: e.g. 1fdaa434827d201370f6c91ad99e9d58510a04a4 says "Replace big string with dictionaries" which was already implemented in a previous commit

#17 Updated by sascha.markus@gmail.com over 1 year ago

  • Assignee changed from sascha.markus@gmail.com to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

Both repos updated with your suggested changes.

I tried to merge two commits into one. But it didn't make it any better :-(
Clearly something I have practice.

#18 Updated by intrigeri over 1 year ago

  • % Done changed from 30 to 60

Code review passes! I'll test soon.

#19 Updated by intrigeri over 1 year ago

Merged this + #7180 into a feature/8514-7180-whisperback-remove-right-pane-and-refactor integration branch (both in tails.git and whisperback.git), built & uploaded a package.

#20 Updated by intrigeri over 1 year ago

  • Assignee changed from intrigeri to sascha.markus@gmail.com
  • QA Check deleted (Ready for QA)

Ouch, I noticed a new blocker! We need to update the "Scenario: tails-debugging-info does not leak information" automated test since the way it parses (yes!) /usr/local/sbin/tails-debugging-info is broken with the new implementation. I think best way to do this would be to first extract the info about all the debug commands, files and directories we want to a machine-readable config file; best would be /etc/whisperback/debugging-info.json (that I could read directly from the test suite that's in Ruby) but worst case, I guess that moving them to a variable in /etc/whisperback/config.py would be OK, as long as one can load that config.py as a library without triggering any side effect.

I recommend an array (to preserve ordering) of arrays. Each such array would contain TYPE and then any type-specific info, e.g.:

[
  ['command', { 'args' => '/usr/sbin/dmidecode', '-s', 'system-manufacturer' }],
  ['file', { 'user': 'root', 'path': '/etc/X11/xorg.conf' }],
  etc.
]

Makes sense?

Once this is implemented on your side I'm happy to adjust the test suite accordingly :)

#21 Updated by intrigeri over 1 year ago

Also, I think we lost the empty line that separates every debugging file/command in the tech details tab: we used to a have a print() in debug_file and friends but now it should be (re-)added to __get_debug_info.

#22 Updated by intrigeri over 1 year ago

And finally, it looks like the improvements from #15468 got lost somewhere along the way. Could you please reapply them wherever it makes sense?

#23 Updated by sascha.markus@gmail.com over 1 year ago

intrigeri wrote:

Ouch, I noticed a new blocker! We need to update the "Scenario: tails-debugging-info does not leak information" automated test since the way it parses (yes!) /usr/local/sbin/tails-debugging-info is broken with the new implementation. I think best way to do this would be to first extract the info about all the debug commands, files and directories we want to a machine-readable config file; best would be /etc/whisperback/debugging-info.json (that I could read directly from the test suite that's in Ruby) but worst case, I guess that moving them to a variable in /etc/whisperback/config.py would be OK, as long as one can load that config.py as a library without triggering any side effect.

I recommend an array (to preserve ordering) of arrays. Each such array would contain TYPE and then any type-specific info, e.g.:

[...]

Makes sense?

Once this is implemented on your side I'm happy to adjust the test suite accordingly :)

The method aleady returns an array of dicts to keep the order. What about adding the info needed for the test to the dict?
Like
[{"type": "file", "path":"/proc/cmdline", "content":["line1", "line2"]}, {"type": "command", "user": "root", "args":["/usr/sbin/dmidecode", "-s", "system-manufacturer"], "content":["line1", "line2"]}, ]

I don't know if we really need args as an array or if it's ok to join them into one string. And if we can unify "path" and "args" to a generic "key"

in the test it would be something like (pseudo ruby ahead...)
info = JSON.parse($vm.file_content("sudo /usr/local/sbin/tails-debugging-info", :user => LIVE_USER)
for i in 0...@info.length
if '#{secret_contents}' == @info[i].content
raise "The secret was leaked by tails-debugging-info via '#{@info[i].path}'"

#24 Updated by sascha.markus@gmail.com over 1 year ago

intrigeri wrote:

And finally, it looks like the improvements from #15468 got lost somewhere along the way. Could you please reapply them wherever it makes sense?

This change and the empty lines mentioned above are only necessary if we want to release whisperback using this branch before we implement #8722.
Is this what we are going to do?

#25 Updated by sascha.markus@gmail.com over 1 year ago

wrote:

intrigeri wrote:

Ouch, I noticed a new blocker! We need to update the "Scenario: tails-debugging-info does not leak information" automated test since the way it parses (yes!) /usr/local/sbin/tails-debugging-info is broken with the new implementation. I think best way to do this would be to first extract the info about all the debug commands, files and directories we want to a machine-readable config file; best would be /etc/whisperback/debugging-info.json (that I could read directly from the test suite that's in Ruby) but worst case, I guess that moving them to a variable in /etc/whisperback/config.py would be OK, as long as one can load that config.py as a library without triggering any side effect.

I recommend an array (to preserve ordering) of arrays. Each such array would contain TYPE and then any type-specific info, e.g.:

[...]

Makes sense?

Once this is implemented on your side I'm happy to adjust the test suite accordingly :)

The method aleady returns an array of dicts to keep the order. What about adding the info needed for the test to the dict?
Like
[{"type": "file", "path":"/proc/cmdline", "content":["line1", "line2"]}, {"type": "command", "user": "root", "args":["/usr/sbin/dmidecode", "-s", "system-manufacturer"], "content":["line1", "line2"]}, ]

I don't know if we really need args as an array or if it's ok to join them into one string. And if we can unify "path" and "args" to a generic "key"

in the test it would be something like (pseudo ruby ahead...)
info = JSON.parse($vm.file_content("sudo /usr/local/sbin/tails-debugging-info", :user => LIVE_USER)
for i in 0...@info.length
if '#{secret_contents}' == @info[i].content
raise "The secret was leaked by tails-debugging-info via '#{@info[i].path}'"

Proposed output

#26 Updated by sascha.markus@gmail.com over 1 year ago

  • Assignee changed from sascha.markus@gmail.com to intrigeri
  • QA Check set to Info Needed

#27 Updated by sascha.markus@gmail.com over 1 year ago

Both repos are updated.
I also added blank lines and #15468

#28 Updated by intrigeri over 1 year ago

intrigeri wrote:

And finally, it looks like the improvements from #15468 got lost somewhere along the way. Could you please reapply them wherever it makes sense?

This change and the empty lines mentioned above are only necessary if we want to release whisperback using this branch before we implement #8722.
Is this what we are going to do?

Yes, plausibly, if it's ready in time for 3.9 but the rest of #8722 is not.

#29 Updated by intrigeri over 1 year ago

  • Assignee changed from intrigeri to sascha.markus@gmail.com
  • QA Check deleted (Info Needed)

Interesting, I had not thought of embedding in the output the info we need for the automated test. This would work and surely would be the cheapest solution to the problem we're tackling, at least on the short term. Now, it feels rather uncommon to run a script (and ask it to do 100% of its job) merely to extract its configuration from its output. That's not a common pattern so I'm a bit worried about how maintainable it will be.

In contrast, the approach I've proposed initially (extracting that config to an external file that can be read by another program without having to run tails-debugging-info) feels much cleaner and less surprising to me.

Now, if you'd rather stick to your proposal, I guess I can live with it, but then please ensure this interface (and the reason why we have it) is properly documented in the script, so nobody breaks this test next time they work on this script again :)

#30 Updated by sascha.markus@gmail.com over 1 year ago

  • Assignee changed from sascha.markus@gmail.com to intrigeri

I made tails-debugging-info configurable and added the configuration.
"user" and "type" are removed again from the output json.
Please have a look if this is OK.
https://github.com/saschamarkus/tails/commit/c6bff024257c86abcbbb220591f430dc9b4aeb56#diff-a0a47c9466b7bed2392053662e153ae8

#31 Updated by intrigeri over 1 year ago

  • QA Check set to Ready for QA

#32 Updated by intrigeri over 1 year ago

  • Assignee changed from intrigeri to sascha.markus@gmail.com
  • QA Check changed from Ready for QA to Info Needed

Looks good but in:

+    for _type, _args in config:
+        if _type == 'command':
+            info.append(debug_command(_args['args'][0], *_args['args'][1:]))
+        else:
+            info.append(debug_file(_args['user'], _args['path']))

… what about the "directory" type?

#33 Updated by sascha.markus@gmail.com over 1 year ago

  • Assignee changed from sascha.markus@gmail.com to intrigeri
  • QA Check changed from Info Needed to Ready for QA

Sorry!
I updated the code. Please pull again.

#34 Updated by intrigeri over 1 year ago

Thanks. I won't have time to look at this before next Wednesday (earliest) but I'm confident we've reached the point where we can merge your work!

#35 Updated by intrigeri about 1 year ago

Merged into feature/8514-7180-whisperback-remove-right-pane-and-refactor in both repos, uploaded a package, will now build an ISO and test!

#36 Updated by intrigeri about 1 year ago

  • Blocked by Bug #15472: Rebase our Tor Browser AppArmor policy on top of torbrowser-launcher 0.2.9-2's added

#37 Updated by intrigeri about 1 year ago

  • QA Check deleted (Ready for QA)

Tested manually, looks good! Next step: update the test suite accordingly.

#38 Updated by intrigeri about 1 year ago

While updating the test suite I noticed that the test was buggy, which is the only reason why it passed so far, because the protection against precisely the attack we're trying to prevent was buggy too. So I'm fixing both the check and the automated test.

#39 Updated by intrigeri about 1 year ago

The test suite now passes and the debug log shows it tests what it should and that the security check works fine. I'll now run the same, fixed test with the security check reverted: it should fail.

#40 Updated by intrigeri about 1 year ago

With 3460524dfc3496f0d5928c663bd0fcd646fb3aae reverted:

  Scenario: tails-debugging-info does not leak information            # features/regression_tests.feature:4
02:06:50.104564115: calling as root: echo 'hello?'
02:06:50.316619034: call returned: [0, "hello?\n", ""]
02:06:51.040808342: [log] CLICK on L(1023,384)@S(0)[0,0 1024x768]
02:06:51.040945968: calling as root: nmcli device show eth0
02:06:51.351496588: call returned: [0, "GENERAL.DEVICE:                         eth0\nGENERAL.TYPE:                           ethernet\nGENERAL.HWADDR:                         50:54:00:F5:93:5C\nGENERAL.MTU:                            1500\nGENERAL.STATE:                          20 (unavailable)\nGENERAL.CONNECTION:                     --\nGENERAL.CON-PATH:                       --\nWIRED-PROPERTIES.CARRIER:               off\n", ""]
02:06:51.361547465: calling as root: date -s '@1530304087'
02:06:51.502432243: call returned: [0, "Fri Jun 29 20:28:07 UTC 2018\n", ""]
    Given I have started Tails from DVD without network and logged in # features/step_definitions/snapshots.rb:186
02:06:51.503677153: opening file /secret in 'append' mode
02:06:51.526681909: append complete
02:06:51.526769812: calling as root: chmod u=rw,go= /secret
02:06:51.758083067: call returned: [0, "", ""]
02:06:51.758241079: calling as root: chown root:root /secret
02:06:51.907644975: call returned: [0, "", ""]
02:06:51.907836918: opening file /etc/whisperback/debugging-info.json in 'read' mode
02:06:52.087468874: read complete
02:06:52.087815936: Config: [["file", {"user"=>"root", "path"=>"/proc/cmdline"}], ["command", {"args"=>["/usr/sbin/dmidecode", "-s", "system-manufacturer"]}], ["command", {"args"=>["/usr/sbin/dmidecode", "-s", "system-product-name"]}], ["command", {"args"=>["/usr/sbin/dmidecode", "-s", "system-version"]}], ["command", {"args"=>["/usr/bin/lspci", "-nn"]}], ["command", {"args"=>["/bin/df", "--human-readable", "--print-type"]}], ["command", {"args"=>["/bin/mount", "--show-labels"]}], ["command", {"args"=>["/bin/lsmod"]}], ["file", {"user"=>"root", "path"=>"/proc/asound/cards"}], ["file", {"user"=>"root", "path"=>"/proc/asound/devices"}], ["file", {"user"=>"root", "path"=>"/proc/asound/modules"}], ["file", {"user"=>"root", "path"=>"/etc/X11/xorg.conf"}], ["file", {"user"=>"Debian-gdm", "path"=>"/var/log/gdm3/tails-greeter.errors"}], ["file", {"user"=>"root", "path"=>"/var/log/live/boot.log"}], ["file", {"user"=>"root", "path"=>"/var/log/live/config.log"}], ["file", {"user"=>"root", "path"=>"/var/lib/live/config/tails.physical_security"}], ["file", {"user"=>"root", "path"=>"/var/lib/gdm3/tails.persistence"}], ["file", {"user"=>"tails-persistence-setup", "path"=>"/live/persistence/TailsData_unlocked/persistence.conf"}], ["file", {"user"=>"tails-persistence-setup", "path"=>"/live/persistence/TailsData_unlocked/live-additional-software.conf"}], ["directory", {"user"=>"root", "path"=>"/live/persistence/TailsData_unlocked/apt-sources.list.d"}], ["file", {"user"=>"root", "path"=>"/var/log/live-persist"}], ["command", {"args"=>["/bin/journalctl", "--catalog", "--no-pager"]}]]
02:06:52.087872226: Looking at ["file", {"user"=>"root", "path"=>"/proc/cmdline"}]
02:06:52.087915515: calling as root: rm /proc/cmdline
02:06:52.223407333: call returned: [1, "", "rm: cannot remove '/proc/cmdline': Operation not permitted\n"]
02:06:52.223565987: Looking at ["command", {"args"=>["/usr/sbin/dmidecode", "-s", "system-manufacturer"]}]
02:06:52.223622803: Looking at ["command", {"args"=>["/usr/sbin/dmidecode", "-s", "system-product-name"]}]
02:06:52.223674205: Looking at ["command", {"args"=>["/usr/sbin/dmidecode", "-s", "system-version"]}]
02:06:52.223704516: Looking at ["command", {"args"=>["/usr/bin/lspci", "-nn"]}]
02:06:52.223764855: Looking at ["command", {"args"=>["/bin/df", "--human-readable", "--print-type"]}]
02:06:52.223792347: Looking at ["command", {"args"=>["/bin/mount", "--show-labels"]}]
02:06:52.223853234: Looking at ["command", {"args"=>["/bin/lsmod"]}]
02:06:52.223881584: Looking at ["file", {"user"=>"root", "path"=>"/proc/asound/cards"}]
02:06:52.223914973: calling as root: rm /proc/asound/cards
02:06:52.392388755: call returned: [1, "", "rm: cannot remove '/proc/asound/cards': Operation not permitted\n"]
02:06:52.392490649: Looking at ["file", {"user"=>"root", "path"=>"/proc/asound/devices"}]
02:06:52.392565555: calling as root: rm /proc/asound/devices
02:06:52.581456508: call returned: [1, "", "rm: cannot remove '/proc/asound/devices': Operation not permitted\n"]
02:06:52.581557948: Looking at ["file", {"user"=>"root", "path"=>"/proc/asound/modules"}]
02:06:52.581617709: calling as root: rm /proc/asound/modules
02:06:52.710104063: call returned: [1, "", "rm: cannot remove '/proc/asound/modules': Operation not permitted\n"]
02:06:52.710201366: Looking at ["file", {"user"=>"root", "path"=>"/etc/X11/xorg.conf"}]
02:06:52.710280450: calling as root: rm /etc/X11/xorg.conf
02:06:52.896137413: call returned: [1, "", "rm: cannot remove '/etc/X11/xorg.conf': No such file or directory\n"]
02:06:52.896245159: Looking at ["file", {"user"=>"Debian-gdm", "path"=>"/var/log/gdm3/tails-greeter.errors"}]
02:06:52.896338943: Looking at ["file", {"user"=>"root", "path"=>"/var/log/live/boot.log"}]
02:06:52.896376295: calling as root: rm /var/log/live/boot.log
02:06:53.071242291: call returned: [0, "", ""]
02:06:53.071343911: calling as root: ln -s /secret /var/log/live/boot.log
02:06:53.255442012: call returned: [0, "", ""]
02:06:53.255584968: calling as root: chown --no-dereference amnesia:amnesia /var/log/live/boot.log
02:06:53.418422573: call returned: [0, "", ""]
02:06:53.418593112: calling as amnesia: sudo /usr/local/sbin/tails-debugging-info | grep 'T0P S3Cr1t -- 3yEs oN1y'
02:06:53.806833191: call returned: [0, "            \"T0P S3Cr1t -- 3yEs oN1y\"\n", ""]
    Then tails-debugging-info is not susceptible to symlink attacks   # features/step_definitions/checks.rb:192
      The secret was leaked by tails-debugging-info via '/var/log/live/boot.log' (RuntimeError)
      ./features/step_definitions/checks.rb:218:in `block (2 levels) in <top (required)>'
      ./features/step_definitions/checks.rb:200:in `/^tails-debugging-info is not susceptible to symlink attacks$/'
      features/regression_tests.feature:6:in `Then tails-debugging-info is not susceptible to symlink attacks'
02:06:56.916210357: calling as root: echo 'hello?'
02:06:57.087678101: call returned: [0, "hello?\n", ""]
02:06:57.087889999: calling as root: journalctl -a --no-pager > /tmp/systemd.journal
02:06:57.257921238: call returned: [0, "", ""]
02:06:57.258049296: opening file /tmp/systemd.journal in 'read' mode
02:07:19.804786684: read complete
      Scenario failed at time 02:06:56

… which demonstrates that the security check was indeed buggy.

#42 Updated by intrigeri about 1 year ago

  • Feature Branch changed from saschamarkus/whisperback:feature/8514-Replace-mail_appended_info-with-a-dictionary,saschamarkus/tails:feature/8514-Replace-WhisperBack.mail_appended_info-with-a-dictionary to feature/8514-7180-whisperback-remove-right-pane-and-refactor,saschamarkus/whisperback:feature/8514-Replace-mail_appended_info-with-a-dictionary

Notes to the reviewer:

  • I'm only asking to review 4e5c9785d149fc4daa73ed6cbc9df55393ed51bc.. on the feature/8514-7180-whisperback-remove-right-pane-and-refactor branch: I've already reviewed the other changes (implemented by Sascha Markus), both in their whisperback.git repo and in tails.git branch; I've imported Sascha's WhisperBack changes via a snapshot package that's imported into the feature-8514-7180-whisperback-remove-right-pane-and-refactor APT overlay which this branch enables; and my branch in tails.git has Sascha's branch merged into it.
  • The only relevant thing you need to know about the other changes is that the list of files/commands/directories printed by tails-debugging-info are now configured in config/chroot_local-includes/etc/whisperback/debugging-info.json.
  • I'm pretty sure I got the logic right wrt. fixing the test case but I still have a slight doubt: it strikes me as odd that anonym could possibly have got it that wrong, so I wonder if I am getting it all wrong myself. That's the main aspect I need another pair of eyes for. The comments on top of config/chroot_local-includes/usr/local/sbin/tails-debugging-info give some context. Sadly, the original bug it's all about (#10333) is private and the other participant on that ticket is MIA currently so I can't ask him his consent to make it public; but the corresponding changelog entry is "Prevent a symlink attack on ~/.xsession-errors via tails-debugging-info which could be used by the amnesia user to read the contents of any file, no matter the permissions", which was mainly implemented in commit 0f8c07ce72e43ff4b48633d6ef45d44c532d8f3d.
  • See the previous comment that demonstrates how the test rightfully fails if I revert my fix to the security check. I'll paste the debug log of the test suite run on the latest version of the branch when I'll submit it for review.
  • The Gherkin (cucumber) text for the affected test is in features/regression_tests.feature. I know the scenario title poorly matches what we're really testing but that's orthogonal to this ticket.

#43 Updated by intrigeri about 1 year ago

Test suite debug log, with the proposed version of this branch:


  Scenario: tails-debugging-info does not leak information            # features/regression_tests.feature:4
02:56:13.018290554: calling as root: echo 'hello?'
02:56:13.299636446: call returned: [0, "hello?\n", ""]
02:56:14.026619006: [log] CLICK on L(1023,384)@S(0)[0,0 1024x768]
02:56:14.027316050: calling as root: nmcli device show eth0
02:56:14.629842871: call returned: [0, "GENERAL.DEVICE:                         eth0\nGENERAL.TYPE:                           ethernet\nGENERAL.HWADDR:                         50:54:00:67:27:BB\nGENERAL.MTU:                            1500\nGENERAL.STATE:                          20 (unavailable)\nGENERAL.CONNECTION:                     --\nGENERAL.CON-PATH:                       --\nWIRED-PROPERTIES.CARRIER:               off\n", ""]
02:56:14.649867138: calling as root: date -s '@1530355259'
02:56:14.995727227: call returned: [0, "Sat Jun 30 10:40:59 UTC 2018\n", ""]
    Given I have started Tails from DVD without network and logged in # features/step_definitions/snapshots.rb:186
02:56:14.998946973: opening file /secret in 'append' mode
02:56:15.039391209: append complete
02:56:15.039652065: calling as root: chmod u=rw,go= /secret
02:56:15.317211542: call returned: [0, "", ""]
02:56:15.317553817: opening file /etc/whisperback/debugging-info.json in 'read' mode
02:56:15.633014137: read complete
02:56:15.633591822: Config: [["file", {"user"=>"root", "path"=>"/proc/cmdline"}], ["command", {"args"=>["/usr/sbin/dmidecode", "-s", "system-manufacturer"]}], ["command", {"args"=>["/usr/sbin/dmidecode", "-s", "system-product-name"]}], ["command", {"args"=>["/usr/sbin/dmidecode", "-s", "system-version"]}], ["command", {"args"=>["/usr/bin/lspci", "-nn"]}], ["command", {"args"=>["/bin/df", "--human-readable", "--print-type"]}], ["command", {"args"=>["/bin/mount", "--show-labels"]}], ["command", {"args"=>["/bin/lsmod"]}], ["file", {"user"=>"root", "path"=>"/proc/asound/cards"}], ["file", {"user"=>"root", "path"=>"/proc/asound/devices"}], ["file", {"user"=>"root", "path"=>"/proc/asound/modules"}], ["file", {"user"=>"root", "path"=>"/etc/X11/xorg.conf"}], ["file", {"user"=>"Debian-gdm", "path"=>"/var/log/gdm3/tails-greeter.errors"}], ["file", {"user"=>"root", "path"=>"/var/log/live/boot.log"}], ["file", {"user"=>"root", "path"=>"/var/log/live/config.log"}], ["file", {"user"=>"root", "path"=>"/var/lib/live/config/tails.physical_security"}], ["file", {"user"=>"root", "path"=>"/var/lib/gdm3/tails.persistence"}], ["file", {"user"=>"tails-persistence-setup", "path"=>"/live/persistence/TailsData_unlocked/persistence.conf"}], ["file", {"user"=>"tails-persistence-setup", "path"=>"/live/persistence/TailsData_unlocked/live-additional-software.conf"}], ["directory", {"user"=>"root", "path"=>"/live/persistence/TailsData_unlocked/apt-sources.list.d"}], ["file", {"user"=>"root", "path"=>"/var/log/live-persist"}], ["command", {"args"=>["/bin/journalctl", "--catalog", "--no-pager"]}]]
02:56:15.633681026: Looking at ["file", {"user"=>"root", "path"=>"/proc/cmdline"}]
02:56:15.633813594: calling as root: rm /proc/cmdline
02:56:15.945785320: call returned: [1, "", "rm: cannot remove '/proc/cmdline': Operation not permitted\n"]
02:56:15.946116569: Looking at ["command", {"args"=>["/usr/sbin/dmidecode", "-s", "system-manufacturer"]}]
02:56:15.946247682: Looking at ["command", {"args"=>["/usr/sbin/dmidecode", "-s", "system-product-name"]}]
02:56:15.946339361: Looking at ["command", {"args"=>["/usr/sbin/dmidecode", "-s", "system-version"]}]
02:56:15.946532795: Looking at ["command", {"args"=>["/usr/bin/lspci", "-nn"]}]
02:56:15.946621089: Looking at ["command", {"args"=>["/bin/df", "--human-readable", "--print-type"]}]
02:56:15.946702524: Looking at ["command", {"args"=>["/bin/mount", "--show-labels"]}]
02:56:15.946791665: Looking at ["command", {"args"=>["/bin/lsmod"]}]
02:56:15.946963300: Looking at ["file", {"user"=>"root", "path"=>"/proc/asound/cards"}]
02:56:15.947063160: calling as root: rm /proc/asound/cards
02:56:16.295140972: call returned: [1, "", "rm: cannot remove '/proc/asound/cards': Operation not permitted\n"]
02:56:16.295370659: Looking at ["file", {"user"=>"root", "path"=>"/proc/asound/devices"}]
02:56:16.295463588: calling as root: rm /proc/asound/devices
02:56:16.593547318: call returned: [1, "", "rm: cannot remove '/proc/asound/devices': Operation not permitted\n"]
02:56:16.593722824: Looking at ["file", {"user"=>"root", "path"=>"/proc/asound/modules"}]
02:56:16.593840472: calling as root: rm /proc/asound/modules
02:56:16.895258477: call returned: [1, "", "rm: cannot remove '/proc/asound/modules': Operation not permitted\n"]
02:56:16.895573441: Looking at ["file", {"user"=>"root", "path"=>"/etc/X11/xorg.conf"}]
02:56:16.895712283: calling as root: rm /etc/X11/xorg.conf
02:56:17.279807229: call returned: [1, "", "rm: cannot remove '/etc/X11/xorg.conf': No such file or directory\n"]
02:56:17.280122850: Looking at ["file", {"user"=>"Debian-gdm", "path"=>"/var/log/gdm3/tails-greeter.errors"}]
02:56:17.280242049: calling as root: rm /var/log/gdm3/tails-greeter.errors
02:56:17.621869129: call returned: [1, "", "rm: cannot remove '/var/log/gdm3/tails-greeter.errors': No such file or directory\n"]
02:56:17.622055409: Looking at ["file", {"user"=>"root", "path"=>"/var/log/live/boot.log"}]
02:56:17.622135824: calling as root: rm /var/log/live/boot.log
02:56:17.978868007: call returned: [0, "", ""]
02:56:17.979061883: calling as root: chown root:root /secret
02:56:18.324554861: call returned: [0, "", ""]
02:56:18.324867650: calling as root: ln -s /secret /var/log/live/boot.log
02:56:18.714077991: call returned: [0, "", ""]
02:56:18.714310120: calling as root: chown --no-dereference amnesia:amnesia /var/log/live/boot.log
02:56:18.994932634: call returned: [0, "", ""]
02:56:18.995206315: calling as amnesia: sudo /usr/local/sbin/tails-debugging-info | grep 'T0P S3Cr1t -- 3yEs oN1y'
02:56:19.694454368: call returned: [1, "", ""]
02:56:19.694643009: calling as root: echo > /var/log/live/boot.log
02:56:19.962151535: call returned: [0, "", ""]
02:56:19.962535622: Looking at ["file", {"user"=>"root", "path"=>"/var/log/live/config.log"}]
02:56:19.962660621: calling as root: rm /var/log/live/config.log
02:56:20.248941286: call returned: [0, "", ""]
02:56:20.249207496: calling as root: chown root:root /secret
02:56:20.525400738: call returned: [0, "", ""]
02:56:20.525607776: calling as root: ln -s /secret /var/log/live/config.log
02:56:20.934072860: call returned: [0, "", ""]
02:56:20.934365506: calling as root: chown --no-dereference amnesia:amnesia /var/log/live/config.log
02:56:21.198729571: call returned: [0, "", ""]
02:56:21.198985041: calling as amnesia: sudo /usr/local/sbin/tails-debugging-info | grep 'T0P S3Cr1t -- 3yEs oN1y'
02:56:21.692277896: call returned: [1, "", ""]
02:56:21.692490589: calling as root: echo > /var/log/live/config.log
02:56:22.015584231: call returned: [0, "", ""]
02:56:22.015800974: Looking at ["file", {"user"=>"root", "path"=>"/var/lib/live/config/tails.physical_security"}]
02:56:22.015941847: calling as root: rm /var/lib/live/config/tails.physical_security
02:56:22.447124825: call returned: [0, "", ""]
02:56:22.447413274: calling as root: chown root:root /secret
02:56:22.890796341: call returned: [0, "", ""]
02:56:22.891076378: calling as root: ln -s /secret /var/lib/live/config/tails.physical_security
02:56:23.168790643: call returned: [0, "", ""]
02:56:23.169107806: calling as root: chown --no-dereference amnesia:amnesia /var/lib/live/config/tails.physical_security
02:56:23.516248759: call returned: [0, "", ""]
02:56:23.516469656: calling as amnesia: sudo /usr/local/sbin/tails-debugging-info | grep 'T0P S3Cr1t -- 3yEs oN1y'
02:56:24.182650065: call returned: [1, "", ""]
02:56:24.182870505: calling as root: echo > /var/lib/live/config/tails.physical_security
02:56:24.489379237: call returned: [0, "", ""]
02:56:24.489645913: Looking at ["file", {"user"=>"root", "path"=>"/var/lib/gdm3/tails.persistence"}]
02:56:24.489772690: calling as root: rm /var/lib/gdm3/tails.persistence
02:56:24.880220848: call returned: [1, "", "rm: cannot remove '/var/lib/gdm3/tails.persistence': No such file or directory\n"]
02:56:24.880411149: Looking at ["file", {"user"=>"tails-persistence-setup", "path"=>"/live/persistence/TailsData_unlocked/persistence.conf"}]
02:56:24.880550597: calling as root: rm /live/persistence/TailsData_unlocked/persistence.conf
02:56:25.273640942: call returned: [1, "", "rm: cannot remove '/live/persistence/TailsData_unlocked/persistence.conf': No such file or directory\n"]
02:56:25.273882978: Looking at ["file", {"user"=>"tails-persistence-setup", "path"=>"/live/persistence/TailsData_unlocked/live-additional-software.conf"}]
02:56:25.273992397: calling as root: rm /live/persistence/TailsData_unlocked/live-additional-software.conf
02:56:25.685055853: call returned: [1, "", "rm: cannot remove '/live/persistence/TailsData_unlocked/live-additional-software.conf': No such file or directory\n"]
02:56:25.685290941: Looking at ["directory", {"user"=>"root", "path"=>"/live/persistence/TailsData_unlocked/apt-sources.list.d"}]
02:56:25.685422676: Looking at ["file", {"user"=>"root", "path"=>"/var/log/live-persist"}]
02:56:25.685496817: calling as root: rm /var/log/live-persist
02:56:26.028899856: call returned: [0, "", ""]
02:56:26.029189872: calling as root: chown root:root /secret
02:56:26.360897282: call returned: [0, "", ""]
02:56:26.361271012: calling as root: ln -s /secret /var/log/live-persist
02:56:26.746349478: call returned: [0, "", ""]
02:56:26.746727843: calling as root: chown --no-dereference amnesia:amnesia /var/log/live-persist
02:56:27.116636010: call returned: [0, "", ""]
02:56:27.116895194: calling as amnesia: sudo /usr/local/sbin/tails-debugging-info | grep 'T0P S3Cr1t -- 3yEs oN1y'
02:56:27.783779219: call returned: [1, "", ""]
02:56:27.784034924: calling as root: echo > /var/log/live-persist
02:56:28.068565847: call returned: [0, "", ""]
02:56:28.068875481: Looking at ["command", {"args"=>["/bin/journalctl", "--catalog", "--no-pager"]}]
    Then tails-debugging-info is not susceptible to symlink attacks   # features/step_definitions/checks.rb:192

#44 Updated by intrigeri about 1 year ago

  • Assignee changed from intrigeri to lamby
  • Estimated time set to 1.00 h
  • QA Check set to Ready for QA

Chris, could you please take the part of this review that's about the work I did on top of Sascha's? See #8514#note-42 to know what to review and what I'm expecting from it. Given you will need to catch up with the surrounding context I think that allocating 1 hour is fair, but if you're faster than that to grasp it all, let me know, I won't complain ;)

I can assign this to someone else if you prefer. I'm proposing it to you first because think you're the person on the FT that is the most experienced wrt. automated testing.

#45 Updated by intrigeri about 1 year ago

#46 Updated by lamby about 1 year ago

Sure thing. Have done some reading and initial review, awaiting response.

#47 Updated by intrigeri about 1 year ago

Sure thing. Have done some reading and initial review,

Great!

awaiting response.

It's not clear to me whom you're waiting a response from, so I'll ignore this unless you tell me I'm needed :)

#48 Updated by lamby about 1 year ago

(Sascha, via github comments. Sorry for being unclear...)

#49 Updated by sascha.markus@gmail.com about 1 year ago

lamby wrote:

(Sascha, via github comments. Sorry for being unclear...)

I received five mails. Three were about the same code in different development stages. I wrote a github comment reply.

One mail was about removing PEP8 changes and applying them in another commit.
I don't see the comment on https://github.com/saschamarkus/whisperback/commit/1fdaa43 , did you remove it?

And one mail was about getting rid of a merge commit. I tried to solve this picking only my commits from
git rebase i feature/8514-Replace-mail_appended_info-with-a-dictionary~10
Now there are some extra commits and the merge is still there :
(

#50 Updated by intrigeri about 1 year ago

Sascha, please don't rewrite your Git history at this point. Sure, it's super messy, but I've already based work on it. You'll do better next time :)

Chris, note that I did not ask you to review Sascha's work.

#51 Updated by sascha.markus@gmail.com about 1 year ago

intrigeri wrote:

Sascha, please don't rewrite your Git history at this point. Sure, it's super messy, but I've already based work on it. You'll do better next time :)

Chris, note that I did not ask you to review Sascha's work.

So I'll ignore the suggestion to change

def __get_debug_info(self, raw_debug)

This will be improved with #8722 Create MIME/Multipart message with attachments in WhisperBack

#52 Updated by lamby about 1 year ago

  • Assignee changed from lamby to intrigeri

I've had a good look at this and I think I understand the private reasons behind 0f8c07ce72e43ff4b48633d6ef45d44c532d8f3d. :) I've reviewed and tested as much as possible, including running the new debugging output script using the JSON-powered stuff.

I remain somewhat lost on the Cucumber test - features/regression_tests.feature seems to contain no tests whatsoever here, just 6 lines of explanatory text.

#53 Updated by intrigeri about 1 year ago

  • Assignee changed from intrigeri to lamby

lamby wrote:

I've had a good look at this and I think I understand the private reasons behind 0f8c07ce72e43ff4b48633d6ef45d44c532d8f3d. :) I've reviewed and tested as much as possible, including running the new debugging output script using the JSON-powered stuff.

Good!

I remain somewhat lost on the Cucumber test - features/regression_tests.feature seems to contain no tests whatsoever here, just 6 lines of explanatory text.

https://git-tails.immerda.ch/tails/tree/features/regression_tests.feature?h=feature/8514-7180-whisperback-remove-right-pane-and-refactor contains one test scenario, written in the Gherkin language used by Cucumber. Then the corresponding steps are defined in features/step_definitions/*.rb. In this case:

git diff --stat origin/devel...origin/feature/8514-7180-whisperback-remove-right-pane-and-refactor
 config/APT_overlays.d/feature-8514-7180-whisperback-remove-right-pane-and-refactor |   0
 config/chroot_local-includes/etc/whisperback/config.py                             |  61 +++++++++++++++++++++-------------------
 config/chroot_local-includes/etc/whisperback/debugging-info.json                   |  24 ++++++++++++++++
 config/chroot_local-includes/usr/local/sbin/tails-debugging-info                   | 126 +++++++++++++++++++++++++++++++++++------------------------------------------------
 features/step_definitions/checks.rb                                                |  21 ++++++++------
 5 files changed, 122 insertions(+), 110 deletions(-)

… the relevant bits are in features/step_definitions/checks.rb: Then /^tails-debugging-info is not susceptible to symlink attacks$/.

Meta: actually, I'd like you to become one more person who at least roughly understands the basics of how our test suite works. So if you feel like it and want to spend 1-2 extra hours on reading a bit about Cucumber and looking at stuff in our features/ directory, by all means, be my guest; I'll gladly bump the "estimated time" value. Starting point: https://tails.boum.org/contribute/release_process/test/automated_tests/ :)

#54 Updated by lamby about 1 year ago

  • Assignee changed from lamby to intrigeri

Thanks for sending over the file in question -- I think I was misled somewhat by the generic filename combined the generic-sounding English tests (indeed, the does not leak information part is actually defined elsewhere).

With regards to getting up to speed on Cuumber, I've spent about an hour reading on it (shall I bump estimated time?) and having a quick play at the same time as trying to get the automated testsuite working locally (although hitting some issues there, but at least generated some related tickets which I've assigned to you, but more systemic issues when trying to run it in a container to get all the right dependencies..).

However, I just think that I have some somewhat-irrational dislike of the BDD method of testing. :/ Probably not the right place to discuss it, but I find it.. I dunno, really really opaque, misleading, ineffective and ugly. I try and have as few "hang-ups" as possible but I fear that BDD is just One Of Those Things.

So whilst I would totally agree we need need to increase the bus factor on the testsuite here I'm not sure I'm 100% the person to do that, especially if someone else is available. I could be persuaded otherwise if there was nobody else, but I just don't think I would be most effective here. Does that make sense? Again, I try to have as few "not touching that" things in tech, but alas... :)

#55 Updated by intrigeri about 1 year ago

With regards to getting up to speed on Cuumber, I've spent about an hour reading on it (shall I bump estimated time?) and having a quick play at the same time as trying to get the automated testsuite working locally (although hitting some issues there, but at least generated some related tickets which I've assigned to you,

Will look into it soon, thanks!

but more systemic issues when trying to run it in a container to get all the right dependencies..).

Not sure if/how libvirt will work in a container. E.g. it uses namespaces itself.

However, I just think that I have some somewhat-irrational dislike of the BDD method of testing. :/ Probably not the right place to discuss it, but I find it.. I dunno, really really opaque, misleading, ineffective and ugly. I try and have as few "hang-ups" as possible but I fear that BDD is just One Of Those Things.

So whilst I would totally agree we need need to increase the bus factor on the testsuite here I'm not sure I'm 100% the person to do that, especially if someone else is available. I could be persuaded otherwise if there was nobody else, but I just don't think I would be most effective here. Does that make sense? Again, I try to have as few "not touching that" things in tech, but alas... :)

OK, thanks for sharing. I'll take a step back, will look 1. at the big picture of our tasks <-> workers mapping; 2. at the "Dogtail+Sikuli's future" matter; and will try to find a solution. Which could be switching to another testing framework entirely with a brighter future and that might suit you better (e.g. OpenQA), or finding someone else to become another Tails test suite developer.

Meanwhile, I'll check with segfault if he's fine with doing this review.

#56 Updated by intrigeri about 1 year ago

  • Assignee changed from intrigeri to segfault

segfault, I'd like a second pair of eyes on this. By this I don't mean the entire branch as I've already reviewed Sascha's work. So before you dive into it, please read:

To be extra clear, the commits I need a review for are:

  • 85cea6e62fc0714fa821187824a87153ca20df65
  • 3460524dfc3496f0d5928c663bd0fcd646fb3aae
  • e50d21a445ff859efc59d2de57da55a27c91e343
  • 32c0fef4098e365302836e663c8b24fd19c50919
  • e50e6e2c8d588f9b46eab9e24de8c9b49820a0a0
  • 59f011ac3e921e04b3aa7f58606df0920bd15a3f

#57 Updated by segfault about 1 year ago

  • Assignee changed from segfault to intrigeri
  • QA Check changed from Ready for QA to Dev Needed

3460524dfc3496f0d5928c663bd0fcd646fb3aae:
The check was not actually meant as a security check, more as a check for whoever adds new files to tails-debugging-info to tell them that adding this file is not secure.

As the comment above the check already says, the check is not sufficient for security (it also only checks the ownership of the file itself, while for security it would also have to check the ownership of all directories).

e50d21a445ff859efc59d2de57da55a27c91e343:
I think there is a misunderstanding. IIUC, the original author of the test (anonym) only wanted to check if, in the case that a file that is writable by amnesia (or any other non-privileged user), so that it could be replaced by a symlink, tails-debugging-info will not print the content of files only readable by root. The test assumes that `debug_file` commands that are called with user=root will also not be susceptible to symlink attacks. So actually what it tests is only whether passing user=amnesia will indeed run `cat` as amnesia.

What you understood, probably because we introduced a (insufficient) permission check when porting the script to Python, is that this test would check for symlink attacks in case that amnesia managed to replace a file that is supposed to be only writable by root with a symlink.

This is also what you implemented now in the test, but since the check is not sufficient to protect against symlink attacks, I don't think it makes sense to test it. I would say we revert your commits, leave the test as it was and explain in a comment that it does assume that the ownership of the target files and all parent directories is described in the docstring of tails-debugging-info.

Later, when we have the time, we can actually add a security check in tails-debugging-info, which we can then write a test for.

#58 Updated by intrigeri about 1 year ago

  • Assignee changed from intrigeri to segfault
  • QA Check changed from Dev Needed to Info Needed

3460524dfc3496f0d5928c663bd0fcd646fb3aae:
The check was not actually meant as a security check, more as a check for whoever adds new files to tails-debugging-info to tell them that adding this file is not secure.

As the comment above the check already says, the check is not sufficient for security (it also only checks the ownership of the file itself, while for security it would also have to check the ownership of all directories).

Indeed, my commit message shows that I did not get this right when I wrote it.

To recap, here's the effect of this commit of mine:

  • If the path is not a symlink, then it's a noop.
  • If the path is a symlink owned by user, then we lose the check on the owner of the symlink target and thus provide less guidance (or none at all) to the developer.
  • If the path is a symlink not owned by user, then we give better guidance to the developer.

To provide at least as good guidance to the developer in all cases, and better guidance in the 3rd case, we would need to check ownership of both the symlink and its target, i.e. something like (untested):

--- a/config/chroot_local-includes/usr/local/sbin/tails-debugging-info
+++ b/config/chroot_local-includes/usr/local/sbin/tails-debugging-info
@@ -103,9 +103,10 @@ def debug_file(user, filename):
 # This check is not sufficient, see the comment at the top of the file
 # for the complete requirements required for security
owner = getpwuid(os.stat(filename, follow_symlinks=False).st_uid).pw_name
-    if owner != user:
+    target_owner = getpwuid(os.stat(filename, follow_symlinks=True).st_uid).pw_name
+    if owner != user or target_owner != user:
return {'key': filename, 'content': '''WARNING: not opening file {}, because it is '''
-                '''owned by {} instead of {}'''.format(filename, owner, user)}
+                '''owned, or is a symlink to a file owned, by {} instead of {}'''.format(filename, owner, user)}

file_content = []
with open(filename) as f:

Would this make sense to you?

e50d21a445ff859efc59d2de57da55a27c91e343:
I think there is a misunderstanding. IIUC, the original author of the test (anonym) only wanted to check if, in the case that a file that is writable by amnesia (or any other non-privileged user), so that it could be replaced by a symlink, tails-debugging-info will not print the content of files only readable by root. The test assumes that `debug_file` commands that are called with user=root will also not be susceptible to symlink attacks. So actually what it tests is only whether passing user=amnesia will indeed run `cat` as amnesia.

What you understood, probably because we introduced a (insufficient) permission check when porting the script to Python, is that this test would check for symlink attacks in case that amnesia managed to replace a file that is supposed to be only writable by root with a symlink.

OK. This makes sense to me. I'm quite pissed off to have spent so much time on this because I was confused by the name of the "tails-debugging-info is not susceptible to symlink attacks" step, which was rather misleading since it did test a tiny subset of what it pretends to, but well, what is done is done :)

This is also what you implemented now in the test, but since the check is not sufficient to protect against symlink attacks, I don't think it makes sense to test it.
I would say we revert your commits, leave the test as it was and explain in a comment that it does assume that the ownership of the target files and all parent directories is described in the docstring of tails-debugging-info.

OK, I'm convinced, let's do something like that. I say "something like" because:

  • As I explained in commit:e50d21a445ff859efc59d2de57da55a27c91e343 (and noticed when I wanted to validate that commit:85cea6e62fc0714fa821187824a87153ca20df65 worked as intended), in its present form this test does not test anything at all. All it does is waste CPU cycles, warm the climate, and make me waste tons of time trying to improve it. So I propose we simply remove it.
  • If for some reason we decide to instead keep this test, then we do need commit:85cea6e62fc0714fa821187824a87153ca20df65 (that's why I started working on this in the first place).

Thank you so much for checking this! I'm very glad a second pair of eyes identified the misunderstanding \o/

#59 Updated by segfault about 1 year ago

  • Assignee changed from segfault to intrigeri

intrigeri wrote:

OK, I'm convinced, let's do something like that. I say "something like" because:

I now understand why the test does not test anymore what it was designed for, i.e. whether debug_file($USER, $FILE) can only read files accessible by $USER:
Since porting tails-debugging-info to Python, we always read the file as root, before it was read as $USER (via sudo -u). I would say that's a bug, and if we fix that, the test would again test what I think it was designed for. I could prepare a commit if you agree.

Indeed.

#60 Updated by intrigeri about 1 year ago

I now understand why the test does not test anymore what it was designed for, i.e. whether debug_file($USER, $FILE) can only read files accessible by $USER:
Since porting tails-debugging-info to Python, we always read the file as root, before it was read as $USER (via sudo -u). I would say that's a bug, and if we fix that, the test would again test what I think it was designed for. I could prepare a commit if you agree.

Wow, indeed the user parameter is now unused (except for the incomplete check we've discussed already). It would be good to fix it some day (ticket++ ?) but let's not block on this now because it's almost orthogonal to this discussion.

That's not why I'm claiming this test does not test anything at all: it does not test anything because "the only files tails-debugging-info displays as non-root simply do not exist in the context where this test is run; they only exist if persistence is enabled or if the Greeter has logged errors. I guess this was not the case back when this test was implemented but that's how it is currently" (from the e50d21a445ff859efc59d2de57da55a27c91e343 message), which I noticed when I implemented 85cea6e62fc0714fa821187824a87153ca20df65 and added debug output to check it worked fine. That's why I'm proposing we remove it.

#61 Updated by intrigeri about 1 year ago

  • Assignee changed from intrigeri to segfault

#62 Updated by segfault about 1 year ago

  • Assignee changed from segfault to intrigeri
  • QA Check changed from Info Needed to Pass

intrigeri wrote:

I now understand why the test does not test anymore what it was designed for, i.e. whether debug_file($USER, $FILE) can only read files accessible by $USER:
Since porting tails-debugging-info to Python, we always read the file as root, before it was read as $USER (via sudo -u). I would say that's a bug, and if we fix that, the test would again test what I think it was designed for. I could prepare a commit if you agree.

Wow, indeed the user parameter is now unused (except for the incomplete check we've discussed already). It would be good to fix it some day (ticket++ ?) but let's not block on this now because it's almost orthogonal to this discussion.

OK.

That's not why I'm claiming this test does not test anything at all: it does not test anything because "the only files tails-debugging-info displays as non-root simply do not exist in the context where this test is run; they only exist if persistence is enabled or if the Greeter has logged errors. I guess this was not the case back when this test was implemented but that's how it is currently"

Right, I completely forgot about that!

(from the e50d21a445ff859efc59d2de57da55a27c91e343 message), which I noticed when I implemented 85cea6e62fc0714fa821187824a87153ca20df65 and added debug output to check it worked fine. That's why I'm proposing we remove it.

OK, lets remove it.

#63 Updated by intrigeri about 1 year ago

  • QA Check changed from Pass to Ready for QA

I've removed the test. Since you did not comment on my proposal to improve test (#8514#note-58) yet and the freeze deadline is now in the past, I've reverted my change so we're back to square one there. I think we're now good to merge. I'll build and test one last time though.

#64 Updated by segfault about 1 year ago

I've removed the test. Since you did not comment on my proposal to improve test (#8514#note-58)

Sorry, I thought that would be something we would do for another release. I will create a ticket for that.

#65 Updated by segfault about 1 year ago

  • Related to Feature #15793: Improve ownership test in tails-debugging-info added

#66 Updated by intrigeri about 1 year ago

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

#67 Updated by intrigeri about 1 year ago

  • Status changed from Fix committed to In Progress

#68 Updated by intrigeri about 1 year ago

  • Status changed from In Progress to Fix committed

#69 Updated by intrigeri about 1 year ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF