Skip to content

Fixes #36759 - only call allowed transpilers#9836

Merged
evgeni merged 1 commit into
theforeman:developfrom
evgeni:i36759
Sep 28, 2023
Merged

Fixes #36759 - only call allowed transpilers#9836
evgeni merged 1 commit into
theforeman:developfrom
evgeni:i36759

Conversation

@evgeni

@evgeni evgeni commented Sep 20, 2023

Copy link
Copy Markdown
Member

CVE-2022-3874: OS command injection via ct_command and fcct_command

Instead of allowing to call any command by changing a setting, only allow specific paths to ct/fcct. If the user needs a different path, they can set it via settings.yaml.

Comment thread db/migrate/20230920093000_move_ct_command_into_own_setting.rb Outdated

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this throws away the currently configured path and I did not add code to set up (fc)ct_location as I think an upgrade warning is sufficient.

@evgeni evgeni force-pushed the i36759 branch 5 times, most recently from 362e8b2 to 194f979 Compare September 20, 2023 09:17
@evgeni

evgeni commented Sep 20, 2023

Copy link
Copy Markdown
Member Author

[test katello]

@ehelms

ehelms commented Sep 20, 2023

Copy link
Copy Markdown
Member

If the user needs a different path, they can set it via settings.yaml.

Will we need to add a set of installer parameters for this?

@evgeni

evgeni commented Sep 20, 2023

Copy link
Copy Markdown
Member Author

We did for sendmail (theforeman/puppet-foreman@6c902a4), so I guess yeah?

ehelms
ehelms previously approved these changes Sep 21, 2023

@ehelms ehelms left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 Not my area of expertise, but in general looks good.

@ekohl want to take a look?

@evgeni

evgeni commented Sep 22, 2023

Copy link
Copy Markdown
Member Author

hmmm, so the code reads correctly, and works inside the console, but the migration doesn't migrate shit

@evgeni

evgeni commented Sep 22, 2023

Copy link
Copy Markdown
Member Author
Validation failed: Name is not allowed to change

Aha!

CVE-2022-3874: OS command injection via ct_command and fcct_command

Instead of allowing to call *any* command by changing a setting, only
allow specific paths to ct/fcct. If the user needs a different path,
they can set it via settings.yaml.
@evgeni

evgeni commented Sep 22, 2023

Copy link
Copy Markdown
Member Author

I hate migrations…

@evgeni evgeni marked this pull request as ready for review September 22, 2023 12:32
@evgeni

evgeni commented Sep 22, 2023

Copy link
Copy Markdown
Member Author

[test katello]

@evgeni evgeni requested review from ehelms and ekohl September 25, 2023 14:34
@evgeni

evgeni commented Sep 26, 2023

Copy link
Copy Markdown
Member Author

release notes: theforeman/theforeman.org#2101

@G-Rath

G-Rath commented Sep 27, 2023

Copy link
Copy Markdown

Just a heads up that this is now being picked up by dependabot as GHSA-9jfq-54vc-9rr2 - if someone can confirm what version this fix is expected to land in, I can prepare a PR to update the advisory with the fixed version :)

Actually digging further, I think the GHSA might be wrong - someone has mixed up forman the gem with foreman the .. app (whatever the not-cli is called 😅)

I've opened github/advisory-database#2761 requesting the advisory be withdrawn.

@evgeni

evgeni commented Sep 28, 2023

Copy link
Copy Markdown
Member Author

@G-Rath thanks. the advisory was withdrawn now.

@evgeni evgeni merged commit d430f3f into theforeman:develop Sep 28, 2023
@evgeni evgeni deleted the i36759 branch September 28, 2023 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants