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.

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

Replace "whitelist" and "blacklist" terms by "allowlist" and "blocklist"/"denylist" #4379

Open
ned64 opened this issue Jun 30, 2021 · 18 comments
Labels
enhancement New feature request

Comments

@ned64
Copy link

ned64 commented Jun 30, 2021

There has been a debate in the IT sec community for a while now about the terms "whitelist" and "blacklist", see e.g. here

dice.com article: ‘Whitelist,’ ‘Blacklist’: The New Debate Over Security Terminology

While certainly not meant by firejail or others in any racial or other negative sense there is at least a potential problem of connotation. "Black" items are to excluded/blocked, while "white" ones will be included/accepted. In a world where there are racism problems in most societies - many of which are about dark and light skin colour - this can be experienced by some as just one more implicit bias of white/black being good/bad.

I therefore suggest that --blacklist options and --whitelist options (including all derivatives) be renamed --blocklist and --allowlist (etc.) as some companies have already started doing.

Alternatively one could use derivatives of allow and deny - terms known from Apache (httpd.conf is where I first came across them decades ago).

In whatever way the change is implemented, the old options would need to be kept (but deprecated a.s.a.p.) as alternative names for existing user configurations not to break, perhaps for quite some time. In all own releases the replacement should be complete, though, including contributed profiles and documentation.

@glitsj16
Copy link
Collaborator

Looks like a duplicate of #3447.

@ned64
Copy link
Author

ned64 commented Jun 30, 2021

@glitsj16 Well spotted. I did search before posting but it didn't turn up.

@glitsj16
Copy link
Collaborator

I did search before posting but it didn't turn up.

@ned64 That does happen sometimes. Especially with 'keywords' like whitelist and blacklist being all over our code base it can help to run a search through the web search engine of your choosing instead of through the GitHub search.

@rusty-snake rusty-snake added the duplicate This issue or pull request already exists label Jun 30, 2021
@netblue30 netblue30 removed the duplicate This issue or pull request already exists label Jul 1, 2021
@netblue30
Copy link
Owner

Tentative move to allow/deny under way. I put a message on the main README.md, and redirected the old #3447 issue here.

@netblue30 netblue30 added the in testing A bugfix that is being tested label Jul 5, 2021
@netblue30
Copy link
Owner

I'm done for now. Everything should be working as before. blacklist/whitelist are set as aliases, nothing should break.

In src/tools/profcleaner.c I have a small tool you can use to convert your local profiles - details in the .c file. Any suggestions in man pages just put them in git. Probably there will be several iterations until we get them right. Hold on on changes throughout the wiki until we release it.

@rusty-snake rusty-snake added the enhancement New feature request label Jul 6, 2021
@netblue30 netblue30 removed the in testing A bugfix that is being tested label Jul 8, 2021
@netblue30
Copy link
Owner

Phase 2

"deny" seems to be working, but "allow" sucks!

I was thinking... In Firefox we have the home directory, which is not a real directory, but kind of a virtual one. So instead of "allow" let's put "virtual". How does it sound? Any other ideas?

Also, do we change file names such as whitelist-common.inc? Git blame/history might get confusing.

@rusty-snake
Copy link
Collaborator

Any other ideas?

whitelist -> allow -> strict-allow

Also, do we change file names such as whitelist-common.inc? Git blame/history might get confusing.

allow-*.inc has already an other use, so we would need to rename these to nodeny-*.inc/nodisable-*.inc.

The thing that makes me worry more it that renaming can break user overrides. We can leave redirect-aliases so include whitelist-*.inc in a .local is this working and we can include the old .locals in the renamed one. But what is with ignore include whitelist-runuser-common.inc for example.

Phase 2

Other notes:

  • CI is still broken
  • a11707e#r53017801 (typo)
  • fe0f975#r53159540 (double spaces)
  • todo: vim syntax
  • Some of the --help message should be improved. Thought --help message need a rework/review in general, I mean --machine-id - preserve /etc/machine-id. --machine-id does the opposite (it creates a new /etc/machine-id).

@netblue30
Copy link
Owner

strict-allow is fine, the best so far!

Let's say we wait until Monday morning in case somebody comes with another idea, and I'll modify the code and the profiles. Then you go in for --help, vim syntax, profile template (we missed it in the first round) and whatever else.

@kmk3
Copy link
Collaborator

kmk3 commented Jul 9, 2021

@netblue30 commented 12 hours ago:

Phase 2

"deny" seems to be working, but "allow" sucks!

I was thinking... In Firefox we have the home directory, which is not a real
directory, but kind of a virtual one. So instead of "allow" let's put
"virtual". How does it sound? Any other ideas?

@netblue30 commented 11 hours ago:

strict-allow is fine, the best so far!

Let's say we wait until Monday morning in case somebody comes with another
idea, and I'll modify the code and the profiles. Then you go in for --help,
vim syntax, profile template (we missed it in the first round) and whatever
else.

Hello, I have a proposal for improving the syntax/semantics of the whitelist
/ allow command (as I also think that it sucks, for multiple reasons) that
I've been meaning to submit for a long time.

I haven't submitted it before because the write-up was a little convoluted and
I wasn't happy with it, but since these changes are being made, I suppose that
now is the time to fix it up and post it.

Anyway, I'll try to do so by Monday (probably on a new issue), so please don't
make any more drastic changes until then.

@rusty-snake
Copy link
Collaborator

FWIW: There was already a discussion about improved semantic in #3447.

@netblue30
Copy link
Owner

Let's keep track of them:

from #4388

  • Update manpages: firejail(1) and firejail-profile(5)
  • Update shell completions
  • Update vim syntax files

@kmk3
Copy link
Collaborator

kmk3 commented Jul 13, 2021

Sorry for not delivering on time. The problems and solutions themselves are
basically defined, but overall the text needs more polish and more connections
between the paragraphs. Also, the current iteration of fixes/suggestions only
came to mind after most of it was written, so I need to refactor some parts.

Here's the current status:

So please bear with me for a few more days (I'm aiming for Friday).

@kmk3
Copy link
Collaborator

kmk3 commented Jul 13, 2021

(Offtopic)

If you also think that this automatic expansion from #issue_number to
[icon issue_title #issue_number](issue url) is a bad idea, please vote on
community/community#4321.

@netblue30
Copy link
Owner

Thanks @kmk3, absolutely no rush! Let's wait until August 1st, and if necessary we move it again.

@kmk3 kmk3 mentioned this issue Jul 13, 2021
@kmk3
Copy link
Collaborator

kmk3 commented Jul 16, 2021

I have made great and steady progress on the text; this is the current status:

  • About 800-900 lines long (I think it's mostly more background and quotes;
    there are some loose/no longer relevant parts to be removed)
  • There are 4-6 extra quotes from #3447 for more background
  • 6/7 sections that come before the solution appear to be finished
  • The solution itself still looks the same, but I need to more explicitly
    mention some advantages
  • Besides the problems, there is 1 new possible enhancement to keep in mind for
    the mid- or long-term

Additionally, today, after copying the quotes and thinking a long time about
some other aspects, it came to mind that noblacklist could be (unless I'm
missing something) rendered completely obsolete by a new (i.e.: not an alias)
command. Now I finally understand why some (seemingly complex/unintuitive)
suggestions were made on #3447. I'm glad to have written down 5
different scenarios since the beginning, as this helps test against what the
result would look like.

The 2 main problems remain solved either way, but this curve ball means that
the solution will have to be adapted for the full set of existing and new
commands to make sense.

So, to avoid further delaying what I think is the most urgent step, I have
started to work on a PR to fix a related issue, which already was the first
action item of my proposal anyway.


(Offtopic)

@kmk3 commented 3 days ago:

(Offtopic)

If you also think that this automatic expansion from #issue_number to
[icon issue_title #issue_number](issue url) is a bad idea, please vote on
github/feedback#4321.

By the way, I have found a workaround for this: manually linking the issues:

Issue [#3447][3447].

[3447]: https://github.com/netblue30/firejail/issues/3447

So funnily enough, the new troublesome non-standard markdown extension has led
me to write markdown in a more standard way (i.e.: avoiding both the old and
the new automatic expansions).

kmk3 added a commit to kmk3/firejail that referenced this issue Jul 18, 2021
This reverts commit fe0f975.

Note: This only reverts the changes from etc.

The 4 aliases introduced on commit 45f2ba5 are mere, well, aliases.
That is, they fail to address the different usability problems discussed
on [netblue30#3447][3447] and in fact only make things more confusing (as has
already been mentioned on [this][4379] and later comments).  The main
reason is that the aliases do not meaningfully map to the original
commands.  For example, the commands from each pair below seem like they
would do the exact same thing:

* `allow` and `nodeny`
* `deny` and `noallow`

Additionally, if these aliases are not the final commands, but only a
test/work-in-progress, then keeping the wide-scale search/replace
changes made on commit fe0f975 would only serve to cause confusion, as
users of firejail-git, contributors and downstream projects might start
changing the commands used on their profiles, only to later have to
change them again, potentially to completely different commands.

The sooner this is undone the better, as (besides the above reasons) the
more profile changes there are between the original commit and the
revert, the harder it is to e.g.: `git diff` versions of files across
the following revision ranges: before the commit, after the commit but
before the revert and after the revert.  Note: This is still the case
even if a commit is [ignored by `git blame`][4390].

So let us revert fe0f975 and only reapply similar large-scale changes
once we have discussed and settled on better commands.

How the revert was applied: Despite using the auto-generated message
from `git revert`, to ensure correctness and to avoid conflicts the
changes were reverted in different steps: Firstly, revert the files
which can be safely reverted directly ("filestorevert"):

    # Find out which files have been changed on fe0f975, but have not
    # been changed afterwards and list them on "filestorevert"
    git show --pretty='' --name-only fe0f975 -- etc | LC_ALL=C sort >allfiles
    git diff --name-only fe0f975..master -- etc | LC_ALL=C sort >filestoignore
    comm -2 -3 allfiles filestoignore >filestorevert

    # Note: There are 3 extra files on filestoignore because they were
    # added after commit fe0f975
    wc -l allfiles filestoignore filestorevert | head -n 3
    #   797 allfiles
    #     8 filestoignore
    #   792 filestorevert

    # Automatically revert files in "filestorevert"
    # See https://stackoverflow.com/a/23401018/10095231
    tr '\n' '\000' <filestorevert | xargs -0 git show fe0f975 -- |
    git apply --reverse

    printf 'Total files reverted:\n'
    git diff --name-only | wc -l
    # 792

Secondly, do some search/replace on the rest:

    tr '\n' '\000' <filestoignore | xargs -0 sed -i.bak \
      -e 's/allow  /whitelist /' -e 's/noallow  /nowhitelist /' \
      -e 's/deny  /blacklist /' -e 's/nodeny  /noblacklist /' \
      -e 's/deny-nolog  /blacklist-nolog /'

    find etc -name '*.bak' -print0 | xargs -0 rm

Thirdly, verify the result.  The following command shows the difference
between all the changes in etc from before fe0f975 and this commit
(inclusive):

    git diff fe0f975~1 -- etc

From the output, it looks like all alias changes are fully reverted and
that the other changes to etc (from after fe0f975) remain, so the
revert seems to be done correctly.

[3447]: netblue30#3447
[4379]: netblue30#4379 (comment)
[4390]: netblue30#4390
@kmk3
Copy link
Collaborator

kmk3 commented Jul 23, 2021

So yesterday I remembered that private-etc and similar commands also suffer
from the same main issue of whitelist, so the Problem 1 section was extended.
Besides that, today I noticed 2 more problems specific to these private-
commands. Thinking about aliases gave me an idea to solve all of this with two
commands instead of over a dozen. These are similar to the other proposed
commands and I think that both sets together would greatly help simplify
whitelisting overall.

Status:

  • About 900-1000 lines long
  • 1 quote added to Problem 1 / Done
  • Problem 1 extended and split into 1.1 (whitelist) and 1.2 (private-etc et
    al) / Done
  • 2 new problems (regarding private-etc et al) / Done
  • 1 new solution (to the above problems) / Mostly done

A few days were spent on #4410 and the rest is basically the same. There are
some adjacent topics (mostly related to previous suggestions and future
changes) that take a lot of space and need organizing that I might just rip out
and only post if they come up.


@kmk3 commented 11 days ago:

Identifies 3 problems with whitelist (and at least one I did not find
mentioned on Replace whitelist and blacklist commands with better terms
#3447)

I was mistaken about this; while I did not find it explicitly described, there
is at least one suggestion in there that takes Problem 1 into consideration.

@kmk3
Copy link
Collaborator

kmk3 commented Aug 2, 2021

  • Problem 1 extended and split into 1.1 (whitelist) and 1.2 (private-etc
    et al) / Done
  • 2 new problems (regarding private-etc et al) / Done

The solution section had many disjoint paragraphs and after trying to also
solve the above, I decided to rethink the commands and redo it Problem by
Problem, step by step. Though not quite done yet, it now makes much more sense
and I think I'll leave out most of what was there.

I have spent a large portion of the time trying out the new commands on
different kinds of (the existing) profiles (e.g.: allow-*.inc,
whitelist-*.inc and normal profiles), to see if they could work and what they
would look like.

This helped a lot to notice redundancies/inconsistencies, so the
private--related commands are different now. This is probably the biggest
change; these commands make more sense now and I think they will be posted as
is.

Some questions came up regarding how whitelisting-related commands, running
multiple commands at the same time and profile organization would work, as the
order of noblacklist and whitelist commands may give different results (not
sure). But since the initial problems are still the same and since this would
also affect prior suggestions from #3447, I'll either just leave this
discussion for the thread or open one separately.

Problem 1 extended and split into 1.1 (whitelist) and 1.2 (private-etc et
al) / Done

  • 2 new problems (regarding private-etc et al) / Done

While the former is still true, the latter was not quite correct and requires
some changes. I had misunterstood the relationship between whitelist and
private- (on keeping vs discarding changes). I think other people might also
be confused by this, so I'll open a discussion later to document this.

On the upside, there are fewer commands proposed now, as many of them were due
to the above, so things are simpler again.

Summary of the changes that I remember:

  • Problem 1.2: Added reference to ed1f0c0
  • Problem 2: Added scenario 6 (noblacklist + nowhitelist)
  • Aliases: Added reference to #4410
  • Solution: Redone (+new private-related commands were changed)

Also, there are some discussions that I intend to open separately:

  • Differences between whitelist and private-
  • Differences between noblacklist and ignore blacklist
  • Add -o option
  • profile.template: dotfiles and misc sorting (mentioned on #4410)

I'd like to open some of them beforehand (even if no answers are posted), to be
able to reference them in the post and if they come up in the comments.

I know it's already past the mentioned August 1st date, but there is still work
to be done. To be safe, I'm aiming for September 1st. If this would block any
ongoing development too much, please let me know.

@samuelfmlourenco

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature request
Projects
None yet
Development

No branches or pull requests

6 participants