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.

Sign up for GitHub

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

Jump to bottom

YETUS-1159. fixes for CVE-2022-24765 #254

Merged
merged 3 commits into from Apr 21, 2022
Merged

YETUS-1159. fixes for CVE-2022-24765 #254

merged 3 commits into from Apr 21, 2022

Conversation

aw-was-here
Copy link
Contributor

@aw-was-here aw-was-here commented Apr 19, 2022 •
edited

This patch does a few things:

  • updates various github actions to the latest versions which help fix things on the Github-side
  • Adds a new yetus_is_container feature routine which helps us figure out if code is running in a container
  • Moves the BASEDIR/.git check to be in routine in common
  • If precommit is running in a container, set GIT_DIR and GIT_CEILING_DIRECTORIES which will prevent git from going past our source tree. We do not set these outside of the container because there is an assumption the user knows what they are doing.
  • Associated documentation

NOTE: this code will almost certainly still fail the Action Test GHA until it is committed to main.

@aw-was-here
YETUS-1159. fixes for CVE-2022-24765
d1e5c87
@aw-was-here
Fixes:
5dd0c7e 
* cannot make check generic since that will break docker-cleanup
* some CI systems need extra help
* mark container as an exec mode for debugging
ndimiduk
ndimiduk approved these changes Apr 21, 2022
View reviewed changes
Copy link
Member

@ndimiduk ndimiduk left a comment

Choose a reason for hiding this comment

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

LGTM. I have just a couple questions.

precommit/src/main/shell/core.d/00-yetuslib.sh
return 0
fi

if [[ -n "${container}" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Where does ${container} come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the reading I've done, lxc will set ${container} to lxc and some other container technologies will also set that to ... something. It's a terrible check, really, but what else can we do? 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added some comments to this code block to hopefully better describe what is going on.

precommit/src/main/shell/core.d/00-yetuslib.sh
return 0
fi

if [[ -d /proc/self/mountinfo ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Is the order of these checks important? Is it better to start with one proc mount vs the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the bit of playing I've done, not really. But I should really optimize the code to use a for loop now that I think about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at this again, I realize why I didn't use a for loop: awk is involved so the quoting gets a bit involved when any of this is in an environment variable.

precommit/src/main/shell/core.d/01-common.sh
{
if [[ ! -e "${BASEDIR}/.git" ]]; then
yetus_error "ERROR: ${BASEDIR} is not a git repo."
cleanup_and_exit 1
Copy link
Member

Choose a reason for hiding this comment

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

Forgive my ignorance. Is it a common pattern in our public API that functions prefixed with verify_ can exit the process when their condition is not met?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question. This one might be the only one. I should probably rename it from verify to ... something else.. since it isn't really a verify function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to check_ which makes a lot more sense.

@aw-was-here
Copy link
Contributor Author

Thanks for review @ndimiduk ! I think I'm going to go ahead and merge this in if only to un-break the action test (hopefully. based upon my local tests it does at least). We can always open more issues or PRs if there is more to do. 😄

@aw-was-here aw-was-here merged commit af54b66 into apache:main Apr 21, 2022
@aw-was-here aw-was-here deleted the test-action branch April 21, 2022 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndimiduk ndimiduk ndimiduk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@aw-was-here @ndimiduk