Template addition #1
Conversation
LaakkonenJussi
force-pushed
the
template_addition
branch
from
6931a4a to
2d4f4f9
Compare
LaakkonenJussi
force-pushed
the
template_addition
branch
from
1686c66 to
7228fb7
Compare
src/firejail/template.c
Outdated
| int key_len; | ||
|
|
||
| if (!tmpl_list) { | ||
| *err = -ENOENT; |
There was a problem hiding this comment.
Is this error reason detail really necessary? Existing code does not use this kind of pattern and even here in the new code that value seems to be ignored except for diagnostic logging.
There was a problem hiding this comment.
I think this is a remnant of code decay here. I had at first an approach to be more lib-like pass the list as parameter but I decided to hide all that functionality.
| if (!str) | ||
| new_string = process_key_value(new_string, token, err); | ||
| else | ||
| new_string = append_to_string(new_string, token); |
There was a problem hiding this comment.
I'm confused about this "starting from second token" thing. What is that all about? [Checking that option/command itself is not a macro has already been checked earlier].
... also: '$' is removed from the 1st token when it looks like the intent might have been to pass it as-is and unmodified?
There was a problem hiding this comment.
Will add check that if there is a lone $ it means an error. If that is any of the internal macros it will be replaced in the string, yeah, not the best way but since I resorted using strtok_r() I guess that is unavoidable unless that is to be done by hand, which may induce more errors..
See also https://github.com/LaakkonenJussi/firejail/pull/1/files#r630917495
There was a problem hiding this comment.
I'd suppose this is resolved but @spiiroin do you still have something that needs to be clarified or do you think this needs fixing? I added comments to code and to commit msg about this as well.
src/firejail/profile.c
Outdated
|
|
||
| if (ptr == NULL) { | ||
| fprintf(stderr, "Ignoring line \"%s\", contains " | ||
| "invalid template keys (err %d)\n", |
There was a problem hiding this comment.
Noting that error code value is not really used by the code itself: as places where those errors are triggered also contain error logging with details -> what are the chances that anybody reading the logs is also interested to know that all that human readable stuff is also known as "(err -127)".
There was a problem hiding this comment.
Maybe I should in general simplify that error use. Existing code does not use much different error codes but I guess here it could be good to report something that describes the actual cause. I did remove most of that to be as internal in template.c for reporting errors, might be enough?
src/firejail/template.c
Outdated
| continue; | ||
| } | ||
|
|
||
| /* Only the first iteration is the template */ |
There was a problem hiding this comment.
Well since this tokenizes first by $ there can be cases of: include /path/to/${template1}/something/${template2}/somewhere ->
1: tokens are a:/path/to, b:{template1}something/ and c:{template2}/somewhere
2: pass only a and b to this functon
3: Only the first item, in b:template1 and c:template2 are considered proper keys, the remains is just added to string.
I have an external test app for checking these cases. So in general when parsing the whole string in no circumstances the string should start in profile/permission file with a template.
There was a problem hiding this comment.
Ok, thanks for the clarification (this is "second level slicer" and you probably meant "2: pass only b and c to this function"?).
If needed, how does one have literal '$' / '{' chars in the final product? I have a feeling that eventually this kind of simple slicing loop will fail to handle something and becomes confusingly complex with special case handling or needs to be replaced with a bit more holistic decode+tokenize logic that allows escaping etc.
There was a problem hiding this comment.
Yes, b and c in that 2. My bad.
I have the process_key_value() implemented in a way that if there is an internal macro it will have its ${INTERNAL_MACRO} format in the string. I'm eager to fix that macro.c to have simpler checker but lets see..
There was a problem hiding this comment.
And I'm not sure how complex this really needs to be. But anyways I haven't worked with Firejail for long so there may be something I've missed but from my point of view the lines in the profiles/permissions do seem to be quite simple.
LaakkonenJussi
force-pushed
the
template_addition
branch
11 times, most recently
from
0180d17 to
af73b1e
Compare
LaakkonenJussi
force-pushed
the
template_addition
branch
2 times, most recently
from
e8761e7 to
8aceebe
Compare
Do not read the profile files when processing arguments. Instead prepend them to a list that is processed after the arguments are processed in order to be able to replace all template keys in the profile file lines. The list is reversed before processing to have the profile files read in the given order. Each item in the list is free'd after it has been processed.
LaakkonenJussi
force-pushed
the
template_addition
branch
3 times, most recently
from
6f6fd8c to
1a2a3ff
Compare
Implement template addition to pass templates as key value pairs as cmd
line parameters to replace the keys in read profile file lines to allow
more customization and flexibility. This adds a new file called
template.c which contains all the functionality.
Motivation for this is to have the possibility to create profile files
where a specific key exists that can be customized per application that
is being run with firejail. For example, application name can be used in
a D-Bus name that is requested to be owned to avoid collision with other
applications requiring the same base name. This can be passed directly
then to firejail as a cmd line parameter when starting the application.
All templates are to be given via cmd line parameters in format:
--template=KEY:VALUE
The keys and values are stored in a single linked list within template.c,
which is free'd when the keys in all read profile file (including
included profiles) lines have been replaced.
Each key can exist only once, existing hardcoded macros cannot be
overwritten. If any of these is violated firejail exits. A template key
cannot start with a digit and must contain alphanumeric chars only.
Each value must conform to following rules:
- length is < 255 (D-Bus name length)
- can contain alphabetical (a-zA-Z), integer (0-9) and '_-/.' chars but
no '..'
In order to use the same DBUS_MAX_NAME_LENGTH it is moved from dbus.c to
firejail.h.
When processing the profile file lines the template keys are expected to
be written as other macros, ${TEMPLATE_KEY}. Template cannot be in the
beginning of the line. If the read line contains other internal macros
they are not replaced as they are processed later with more strict and
specific checks. It is known that using strtok_r() and doing the
tokenization in two steps, first by '$' and then by '{}' invalid
definitions such as ${{TEMPLATE_KEY2}} will pass the checks. The process
of replacing the keys can be described as follows to ease the
understanding of the code:
1. "whitelist ${HOME}/${key1}/path/to/${key2}.somewhere" tokens are:
a: whitelist
b: {HOME}/
c: {key1}/path/to/
d: {key2}.somewhere
2. Keys in the first token 'a' are ignored, it is the start of new str
3. Tokens 'b', 'c' and 'd' are passed to process_key_value
4. Each of the template keys are replaced with corresponding values, as
${HOME} is internal macro it is not replaced but added as is. Only the
first items in tokens, 'key1' and 'key2' are considered as proper keys to
have the values replaced, the remains are just added to the str.
5. Resulting string would be then:
"whitelist ${HOME}/value1/path/to/value2.somewhere"
In order to avoid unnecessary duplication of each read profile line the
line is first checked to have at least one template key. If the template
key is not found firejail will exit with an error.
Man pages for firejail and firejail-profile are updated to include this
addition.
LaakkonenJussi
force-pushed
the
template_addition
branch
from
1a2a3ff to
87906fc
Compare
No description provided.