Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ensure safe perms for svs.sqlite and sync_flag #1256

Merged
merged 1 commit into from
May 11, 2021
Merged

ensure safe perms for svs.sqlite and sync_flag #1256

merged 1 commit into from
May 11, 2021

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented Apr 29, 2021 •
edited by emkll
Loading

Description

Fixes #1232

Test Plan

  • Build deb for the client and install on sd-app or just patch it with this PR change
  • Delete ~/.securedrop_client (you can manually copy config.json if you want but it's not necessary to test this change)
  • Start the client via commandline by running: securedrop-client
  • Confirm svs.sqlite and sync_flag now have 600 perms instead of 644

@sssoleileraaa sssoleileraaa requested a review from a team as a code owner April 29, 2021 18:49
@sssoleileraaa sssoleileraaa force-pushed the tighten-perms-on-sqlite-file branch from 8b7e3ab to f3d2732 Compare April 29, 2021 18:58
@emkll emkll force-pushed the tighten-perms-on-sqlite-file branch from f3d2732 to b3f16cc Compare April 30, 2021 18:15
@emkll
Copy link
Contributor

emkll commented Apr 30, 2021

rebased on latest main

emkll
emkll previously approved these changes May 4, 2021
View reviewed changes
Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes here work well, but it's important to note that it will not update existing svs.sqlite and sync_flag files: These changes will only work if/when those files are created by the client. I think this is fine to merge as-is.

@sssoleileraaa sssoleileraaa force-pushed the tighten-perms-on-sqlite-file branch from b3f16cc to 38a1d2e Compare May 5, 2021 23:32
@sssoleileraaa
Copy link
Contributor Author

@emkll - to address your feedback, i updated the client to fix insecure perms when it starts. follow the test plan, but instead of deleting ~/.securedrop_client just modify the permissions of the files to something like 644 to test this new behavior.

@sssoleileraaa sssoleileraaa force-pushed the tighten-perms-on-sqlite-file branch 2 times, most recently from a196e85 to 00a9c08 Compare May 6, 2021 00:57
@sssoleileraaa sssoleileraaa force-pushed the tighten-perms-on-sqlite-file branch from 00a9c08 to 8019c11 Compare May 6, 2021 01:08
emkll
emkll approved these changes May 11, 2021
View reviewed changes
Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @creviera for the changes, went through another round of testing, looks good to me!

@emkll emkll merged commit 09621da into main May 11, 2021
@emkll emkll deleted the tighten-perms-on-sqlite-file branch May 11, 2021 15:44
@sssoleileraaa sssoleileraaa mentioned this pull request Aug 16, 2021
Closed
5 tasks
@eloquence eloquence mentioned this pull request Sep 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emkll emkll emkll approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tighten svs.sqlite file permissions
2 participants
@sssoleileraaa @emkll