exit: validate cid. - #53
Conversation
Reject only empty, `.`, `..`, `/`, and NUL so exit_dir writes cannot escape while still accepting previously valid container IDs. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
|
Ephemeral COPR build failed. |
| /// allow escaping `exit_dir` or confusing path APIs. Other characters (including | ||
| /// `+`, `:`, and non-ASCII) are accepted so historically valid container IDs keep | ||
| /// working. | ||
| fn is_safe_container_id(cid: &str) -> bool { |
There was a problem hiding this comment.
This is perfectly fine.
Just one thing to consider if the cid is used in multiple places is the newtype/Make Illegal States Unrepresentable idiom, i.e. creating a wrapper type for Cid and making sure that if you are able to construct it, it is a valid ID.
Something like (there are many traits that can be implemented):
pub struct Cid(String);
use std::str::FromStr;
impl FromStr for Cid {
type Err = conmon::error::ConmonError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
// Would be inlined.
if !is_safe_container_id(cid) {
return Err(ConmonError::new("invalid cid");
}
Ok(Cid(s.to_string()))
}
}See:
- https://corrode.dev/blog/illegal-state/
- https://web.archive.org/web/20230519162111/https://www.worthe-it.co.za/blog/2020-10-31-newtype-pattern-in-rust.html
- https://rust-unofficial.github.io/patterns/patterns/behavioural/newtype.html
I'll be glad to rewrite it in that style in a new PR, if I'm being too annoying:)
| /// `+`, `:`, and non-ASCII) are accepted so historically valid container IDs keep | ||
| /// working. | ||
| fn is_safe_container_id(cid: &str) -> bool { | ||
| !cid.is_empty() && cid != "." && cid != ".." && !cid.contains('/') && !cid.contains('\0') |
There was a problem hiding this comment.
One thing I noticed is that the CommonCfg implements Default and it the cid field defaults to an invalid ID (the empty string).
| } | ||
|
|
||
| #[test] | ||
| fn write_exit_files_does_not_escape_exit_dir() { |
There was a problem hiding this comment.
To be extra safe, write_exit_files could use Path::starts_with or similar to validate that the path doesn't escape the directory. Not that it's needed currently.
simek-m
left a comment
simek-m
left a comment
There was a problem hiding this comment.
It looks good and the tests cover a lot, only a stylistic comment as usual:) LGTM.
Invalid container IDs are unrepresentable in CommonCfg and write_exit_files(). They are validated at startup. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
|
One thing regarding the newtype I forgot - in a perfect world, it should be in its own module to enforce the validation, see https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=9fa00c7f638311a8068636663be657f8 |
2 participants
Reject only empty,
.,..,/, and NUL so exit_dir writes cannot escape while still accepting previously valid container IDs.