Validate DHCP client identity and source address - #191
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8aeb89bf97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| CFString::new("dhcp_ignore_client_identifier"), | ||
| CFNumber::from(1_i32), |
There was a problem hiding this comment.
When a DHCP client supplies or rotates option 61, this value is written to the plist as a numeric CFNumber, but bootpd defines dhcp_ignore_client_identifier as a Boolean preference and does not coerce an integer into that setting. The flag therefore remains ineffective, allowing a client to obtain leases under multiple identifiers and potentially exhaust the address pool; construct this entry with CFBoolean::true_value() instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This does not appear to be an issue.
bootpd loads this property with SET_NUMBER_FROM_PLIST, and it explicitly accepts a CFNumber.
But I agree that using CFBoolean is more sound here, fixing in 30787c8.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30787c8c67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| SCPreferencesCommitChanges(prefs.as_concrete_TypeRef()); | ||
| anyhow::ensure!( | ||
| SCPreferencesCommitChanges(prefs.as_concrete_TypeRef()) != 0, |
There was a problem hiding this comment.
When multiple VMs start concurrently, each process opens an unlocked SCPreferences session for the same plist, so one process can commit after another session was created and cause the latter's SCPreferencesCommitChanges to fail with stale preferences. Unlike the previous code, this failure now propagates out of try_main and prevents that VM's proxy from starting even when both processes are writing identical defaults. Lock the preferences before mutation/commit or synchronize and retry stale commits before treating the result as fatal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6595a6c226
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ), | ||
| ( | ||
| CFString::new("dhcp_ignore_client_identifier"), | ||
| CFBoolean::true_value().as_CFType(), |
There was a problem hiding this comment.
The fresh evidence in the current diff is that this setting has now been changed to CFBoolean, whereas bootpd loads dhcp_ignore_client_identifier through SET_NUMBER_FROM_PLIST, which expects a CFNumber; Core Foundation booleans have a different type ID and are therefore ignored. When a client changes option 61, bootpd will continue allocating leases by client identifier, defeating the intended protection against exhausting the DHCP pool; encode 1 as a CFNumber instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
SET_NUMBER_FROM_PLIST does not require a CFNumber, see #191 (comment) for a proof.
* Require BOOTP chaddr match for host→VM DHCP responses Mirror #191 request-path identity checks: admit/forward DHCP BootReplies only when chaddr matches the VM MAC, so foreign client replies are not written into the guest fd. Co-authored-by: Cursor <cursoragent@cursor.com> * $ cargo fmt --------- Co-authored-by: genforAI <genforAI@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Nikolay Edigaryev <edi@openai.com>
2 participants
This improves DHCP-path packet validation and hardens
bootpd(8)configuration.Thanks to Darryl Jaskolski for finding and reporting these issues!