config.c: enforce stricter parsing of config files#427
config.c: enforce stricter parsing of config files#427cgzones merged 1 commit intologrotate:masterfrom
Conversation
|
I have not yet looked at the web pages you refer to but we always need to balance. What about compatibility across logrotate versions? If a logrotate config file gets updated to use an option that is not yet supported by the installed version of logrotate, should it prevent logs from being rotated? |
|
The PR contains two changes:
1 should not have any negative compatibility side effects, but you are right that 2 breaks forward compatibility when a new keyword is introduced. I personally would prefer not rotating logfiles to potentially incorrect rotation when an unsupported option is used, but I guess that depends on a lot of external factors. How about changing config.c#L1113 to a hard error instead? This would still allow unknown keywords as long as they are followed by a space or = sign so valid config files of future versions should not be an issue. But it would still make it harder to trick the parser into parsing a random file as a valid config. WDYT? |
|
Sorry for the delay! Yes, it makes sense to me. Unknown configuration directive should trigger a warning only whereas lexically incorrect garbage should trigger an error and prevent the remainder of the configuration file from being taken into account. |
40fa908 to
34a7c3b
Compare
Abort parsing of config files that contain invalid lines. This makes it harder to abuse logrotate for privilege escalation attacks where an attacker can partially control a privileged file write.
34a7c3b to
e9fa963
Compare
|
Thanks Kamil. |
|
@kdudka friendly ping. |
|
Sorry for the delay! I was on vacation last week. The proposed changes look fair to me. I would appreciate an additional review by @cgzones though. He is the original author of the |
cgzones
left a comment
There was a problem hiding this comment.
Please add test cases for the following configurations:
&DIR&/test1.log {
newkeyword
}
&DIR&/test2.log {
rotate 1
}
and check that test1.log has not been rotated, but test2.log.
&DIR&/test1.log {
g@rbag€[]+#*
}
&DIR&/test2.log {
rotate 1
}
and check that both test1.log and test2.log have not been rotated.
|
|
||
| $RLR test-config.102 --force | ||
|
|
||
| if [ $? -eq 0 ]; then |
There was a problem hiding this comment.
Please also check that test.log has not been rotated.
|
|
||
| $RLR test-config.103 --force | ||
|
|
||
| if [ $? -eq 0 ]; then |
There was a problem hiding this comment.
Please also check that test.log has not been rotated.
|
@cgzones My proposal was actually not to treat unknown configuration directives as errors that would prevent log files from being rotated. logrotate configuration files are often updated independently of logrotate itself and their compatibility with older logrotate releases is important in my opinion. |
|
Definitely. Thank you for taking care of it! |
|
Applied, thanks for the contribution. |
|
Thanks for pushing this through! 👍 |
A number of recent exploits have abused logrotate's lax parsing of config files as an easy way to turn privileged file write bugs into code execution:
These exploits depend on bugs in other parts of the OS (apport, kernel, ..) to write partially controlled files to the logrotate config directory. Targeting logrotate is highly useful for attackers, because it will ignore invalid parts of config files and continue parsing until it finds a valid configuration.
This behaviour makes exploitation of tricky bugs such as the linked coredump vulnerabilities much easier.
This PR enforces stricter parsing of config files, by turning some of the already existing parser warnings into hard errors. It does not have any effect on users using valid configuration files, but might be a breaking change for people relying on the current handling of invalid files. As current versions already print an error message when encountering such files, no widely used configurations should be affected.
Overall, I think this would be a nice defense-in-depth change to improve the security of many downstream logrotate users.