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

"yes to all" does nothing on qrexec service call with specific argument #2403

Open
marmarek opened this Issue Oct 28, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@marmarek
Member

marmarek commented Oct 28, 2016

Qubes OS version (e.g., R3.1):

R3.2

Expected behavior:

"always allow" permission is saved for this particular service argument

Actual behavior:

Nothing happens.

Steps to reproduce the behavior:

Call a service with some argument.

General notes:

Reported by @Rudd-O

if I say "yes to all" when there is an argument
(it's already in the code I just pushed to Github) then nothing actually
gets saved as "yes to all" anywhere. In other words: there's a bug as
"Yes to all" does nothing when an argument is specified.

@marmarek

This comment has been minimized.

Show comment
Hide comment
@marmarek

marmarek Oct 30, 2016

Member

Automated announcement from builder-github

The package qubes-core-dom0-linux-3.2.8-1.fc23 has been pushed to the r3.2 testing repository for dom0.
To test this update, please install it with the following command:

sudo qubes-dom0-update --enablerepo=qubes-dom0-current-testing

Changes included in this update

Member

marmarek commented Oct 30, 2016

Automated announcement from builder-github

The package qubes-core-dom0-linux-3.2.8-1.fc23 has been pushed to the r3.2 testing repository for dom0.
To test this update, please install it with the following command:

sudo qubes-dom0-update --enablerepo=qubes-dom0-current-testing

Changes included in this update

@Rudd-O

This comment has been minimized.

Show comment
Hide comment
@Rudd-O

Rudd-O Oct 30, 2016

I think this fix is wrong. I have detailed what I think the error is here, as well as an outline of what should be done instead: marmarek/qubes-core-admin-linux@1dff636#commitcomment-19626640

Rudd-O commented Oct 30, 2016

I think this fix is wrong. I have detailed what I think the error is here, as well as an outline of what should be done instead: marmarek/qubes-core-admin-linux@1dff636#commitcomment-19626640

@marmarek marmarek reopened this Oct 31, 2016

@marmarek

This comment has been minimized.

Show comment
Hide comment
@marmarek

marmarek Oct 31, 2016

Member

As discussed below the commit, copying generic policy to the argument specific one, at the time of first "yes to all" isn't the best idea. Better would be including generic one from specific one. But this must be done explicitly to avoid unwanted side effects.

Assumptions and constrains

  1. Behavior of already existing configuration cannot change. Especially, every policy file have implicit "deny all".
  2. It should be obvious what and when will be processed, without referring to specification. Explaining things in (reasonably short) comments in the file itself is ok.
  3. It should be defensive by design. Better have spurious deny in case of user error, than spurious allow.

Proposal 1

Add $include:some-name syntax, as the only entry in the line. Some name would be a file name in /etc/qubes-rpc/policy (or more formally: in directory with qrexec policy). This statement would continue processing policy to the named file. Because of implicit deny at the file end, it makes sense only as the last entry - entries after that would be after "end of included file", so after implicit "deny all". Implementation should throw a warning if any non-comment is found after such statement. When such files are created automatically, should contain a comment "entries after $include:... are ignored".
Maybe better name it somehow else than $include to better suggest this behavior?

Proposal 2

Similar to "Proposal 1", but without implicit deny at the end of included file. On the one hand it will be more flexible, but on the other hand it makes the file have implicit deny and sometime have not, depending on how the file is loaded. This somehow violates requirement no 2. But at the same time, it's better match for keyword $include:....

Member

marmarek commented Oct 31, 2016

As discussed below the commit, copying generic policy to the argument specific one, at the time of first "yes to all" isn't the best idea. Better would be including generic one from specific one. But this must be done explicitly to avoid unwanted side effects.

Assumptions and constrains

  1. Behavior of already existing configuration cannot change. Especially, every policy file have implicit "deny all".
  2. It should be obvious what and when will be processed, without referring to specification. Explaining things in (reasonably short) comments in the file itself is ok.
  3. It should be defensive by design. Better have spurious deny in case of user error, than spurious allow.

Proposal 1

Add $include:some-name syntax, as the only entry in the line. Some name would be a file name in /etc/qubes-rpc/policy (or more formally: in directory with qrexec policy). This statement would continue processing policy to the named file. Because of implicit deny at the file end, it makes sense only as the last entry - entries after that would be after "end of included file", so after implicit "deny all". Implementation should throw a warning if any non-comment is found after such statement. When such files are created automatically, should contain a comment "entries after $include:... are ignored".
Maybe better name it somehow else than $include to better suggest this behavior?

Proposal 2

Similar to "Proposal 1", but without implicit deny at the end of included file. On the one hand it will be more flexible, but on the other hand it makes the file have implicit deny and sometime have not, depending on how the file is loaded. This somehow violates requirement no 2. But at the same time, it's better match for keyword $include:....

@Rudd-O

This comment has been minimized.

Show comment
Hide comment
@Rudd-O

Rudd-O Oct 31, 2016

Cool! I'll take a look at this again when I wake up.

Rudd-O
http://rudd-o.com/

Rudd-O commented Oct 31, 2016

Cool! I'll take a look at this again when I wake up.

Rudd-O
http://rudd-o.com/
@qubesos-bot

This comment has been minimized.

Show comment
Hide comment
@qubesos-bot

qubesos-bot Jan 8, 2017

Automated announcement from builder-github

The package qubes-core-dom0-linux-3.2.11-1.fc23 has been pushed to the r3.2 stable repository for dom0.
To install this update, please use the standard update command:

sudo qubes-dom0-update

Or update dom0 via Qubes Manager.

Changes included in this update

Automated announcement from builder-github

The package qubes-core-dom0-linux-3.2.11-1.fc23 has been pushed to the r3.2 stable repository for dom0.
To install this update, please use the standard update command:

sudo qubes-dom0-update

Or update dom0 via Qubes Manager.

Changes included in this update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment