Skip to content
This repository has been archived by the owner on Jan 4, 2024. It is now read-only.

handle 'date' and 'main' lookups, expand 'env' #2

Merged
merged 4 commits into from Dec 17, 2021

Conversation

awortman-fastly
Copy link
Collaborator

@awortman-fastly awortman-fastly commented Dec 16, 2021

The logic for these lookups is simple and lossy, but that's acceptable
since we can at least report that these lookups will be made. For some
users it is reasonable to simply reject a log line at the presence of
main or date, regardless of later findings. For other users, it
doesn't seem possible to have a general transformation between a main
or date lookup, and a string useful to build other more concerning
lookup tokens (like jndi or env).

edit: also adjusted the env expansion to assume that env vars are for undefined variables (so, expand to ""), but use a default value if one is provided - ${env:UNDEFINED:-hello} should expand to hello.

The logic for these lookups is simple and lossy, but that's acceptable
since we can at least report that these lookups will be made. For some
users it is reasonable to simply reject a log line at the presence of
`main` or `date`, regardless of later findings. For other users, it
doesn't seem possible to have a general transformation between a `main`
or `date` lookup, and a string useful to build other more concerning
lookup tokens (like `jndi` or `env`).
@awortman-fastly awortman-fastly changed the title handle 'date' and 'main' lookups handle 'date' and 'main' lookups, expand 'env' Dec 16, 2021
Copy link
Collaborator

@sw17ch sw17ch left a comment

Choose a reason for hiding this comment

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

two mostly-questions that might be mistakes.

src/lib.rs Outdated
//
// Report the `main` handler for the possible information disclosure risk, then assume
// empty string expansion in case this was just obfuscatory.
("".into(), Some(Handler::Main))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be default.into()? I'm not entirely sure of the semantics around main. What happens in the case of ${main:blahblah:-jndi}?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this should be default.into(), i incorrectly thought main did not support default value syntax.

// select substrings, so month/day strings aren't useful for building an unaccepatble
// handler. Assume that if `${date:...}` is showing up in an interesting place, it will be
// with some format string that expands to empty string.
("".into(), Some(Handler::Date))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question here as for main: should this be default.into()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

date doesn't seem to support the default value syntax, so a bad substitution ends up being the empty string. i've been consulting https://logging.apache.org/log4j/2.x/manual/lookups.html#AppMainArgsLookup and looking for default around where the lookup is defined to see if's permitted or not (haven't found where in the source default values are actually interpolated..)

@awortman-fastly awortman-fastly merged commit d91a60a into main Dec 17, 2021
Copy link
Collaborator

@sw17ch sw17ch left a comment

Choose a reason for hiding this comment

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

Retroactive approved.

@sw17ch sw17ch deleted the ixi/main-and-env-handlers branch December 17, 2021 01:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants