Address challenge feedback from Discord - #242
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: a9157c5862
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for insn in disas[read_i + 1:write_i] | ||
| if instruction_sets_reg(insn, "rdx", "rax") | ||
| ] | ||
| assert rdx_after_read, ( |
There was a problem hiding this comment.
When a solution does mov rdx, rax after the read and then overwrites rdx before the write syscall, this new check still passes because it only requires that the read-return copy appears somewhere between the two syscalls. check_runtime only asserts the program exits with code 42, so a program can pass while writing zero bytes or a hardcoded length instead of printing exactly the flag; keep tracking rdx writes and require the last one before write to come from rax, as the shared helper did.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
THIS MESSAGE IS GENERATED BY AN AUTOMATED PROCESS.
Addressed this feedback and pushed commit c24b9c84.
Original feedback: #242 (comment)
Automated run notes:
Changed challenges/computing-101/hello-hackers/open-read-write-rip-relative/challenge/.py/chal.py so the challenge-local assert_write_count_from_read tracks all recognized rdx writes between the read and write syscalls and requires the last one before write to set rdx from rax.
This closes the bypass where mov rdx, rax appeared after read but rdx was overwritten before write.
No separate repo change was needed for the parent review entry; the only actionable instruction in that review was the inline comment above.
Validation:
- Ran
nix develop --command pwnshop test challenges/computing-101/hello-hackers/open-read-write-rip-relative; it passed with 1 challenge and 1 testcase. - Ran a focused synthetic-disassembly check against the helper confirming a valid read-to-write
rdxflow still passes and the stale-rdxsequence now raisesAssertionError.
|
THIS MESSAGE IS GENERATED BY AN AUTOMATED PROCESS. In response to #242 (review): Addressed this feedback and pushed commit Original feedback: #242 (review) Automated run notes: Changed No separate repo change was needed for the parent review entry; the only actionable instruction in that review was the inline comment above. Validation:
|
| **THIS IS STILL NOT WHAT WE WANT!** | ||
| Why? Because it _reads_ the 8 bytes at `[rip+path]` into `rdi` rather than put the address of those bytes into `rdi`. | ||
| `rdi` would end up holding the values `'f'`, and `'l'`, and so on, but the `open` syscall needs the address and not the values. | ||
| Think of `path` as the address where the first byte of `"/flag\0"` lives: `mov` copies bytes from there, while `lea` copies the address so the kernel can walk those bytes until the null byte. |
There was a problem hiding this comment.
How did you put lea before the place where we freaking introduce it? OBVIOUSLY this sentence should be later. Like, it's insane that I have to type this; edit whatever relevant skill to fix it so you don't make this mistake again.
There was a problem hiding this comment.
THIS MESSAGE IS GENERATED BY AN AUTOMATED PROCESS.
Addressed this feedback and pushed commit e82c47c0.
Original feedback: #242 (comment)
Automated run notes:
Feedback Addressed
Moved the mov/lea comparison in challenges/computing-101/hello-hackers/open-read-write-rip-relative/DESCRIPTION.md until after lea is introduced.
Also updated .agents/skills/authoring-challenges/SKILL.md with a local-ordering rule so future DESCRIPTION edits do not use an instruction, term, or comparison before introducing it.
Removed the added envp[] pointer-table wording from challenges/computing-101/the-stack-revisited/gdb-stack-align-2/DESCRIPTION.md.
The file now matches origin/main for this item.
Removed the confusing envp[] pointer-table sentence from challenges/computing-101/the-stack-revisited/mem-stack-align/DESCRIPTION.md.
Kept the direct clarification that the environment string includes FOO=, the value, and the trailing null byte.
Restored all challenges/web-security/** files to origin/main, including deleting the PR-added challenges/web-security/common/Dockerfile.j2.
The effective diff against origin/main now contains no web-security changes.
Validation
git diff --check: clean.
nix develop --command pwnshop test challenges/computing-101/hello-hackers/open-read-write-rip-relative: passed, 1 challenge / 1 testcase.
No web-security tests were run because the requested resolution was to remove all web-security changes from the PR.
| ``` | ||
|
|
||
| This allows you to have very finegrained control over your environment. | ||
| When you build that one variable, count the bytes in its environment string (`NAME=`, the value, and its trailing null byte), not a separate `envp[]` pointer-table slot. |
There was a problem hiding this comment.
This seems to force confusion into the description. What are you trying to achieve here? If it's relevant, don't do it in a way that confuses the learner further.
There was a problem hiding this comment.
THIS MESSAGE IS GENERATED BY AN AUTOMATED PROCESS.
Addressed this feedback and pushed commit e82c47c0.
Original feedback: #242 (comment)
Automated run notes:
Feedback Addressed
Moved the mov/lea comparison in challenges/computing-101/hello-hackers/open-read-write-rip-relative/DESCRIPTION.md until after lea is introduced.
Also updated .agents/skills/authoring-challenges/SKILL.md with a local-ordering rule so future DESCRIPTION edits do not use an instruction, term, or comparison before introducing it.
Removed the added envp[] pointer-table wording from challenges/computing-101/the-stack-revisited/gdb-stack-align-2/DESCRIPTION.md.
The file now matches origin/main for this item.
Removed the confusing envp[] pointer-table sentence from challenges/computing-101/the-stack-revisited/mem-stack-align/DESCRIPTION.md.
Kept the direct clarification that the environment string includes FOO=, the value, and the trailing null byte.
Restored all challenges/web-security/** files to origin/main, including deleting the PR-added challenges/web-security/common/Dockerfile.j2.
The effective diff against origin/main now contains no web-security changes.
Validation
git diff --check: clean.
nix develop --command pwnshop test challenges/computing-101/hello-hackers/open-read-write-rip-relative: passed, 1 challenge / 1 testcase.
No web-security tests were run because the requested resolution was to remove all web-security changes from the PR.
|
|
||
| Remember that the whole environment string is placed on the stack, so `FOO=` and the trailing null byte count toward the shift too. | ||
| Remember that the whole environment string is placed on the stack, so `FOO=`, the value, and the trailing null byte count toward the shift. | ||
| Count those string bytes, not a separate `envp[]` pointer-table slot. |
There was a problem hiding this comment.
Again, where did envp pointer-table slot come in? This just confuses the learner! If there is a point to this, make it in a way that's not confusing.
There was a problem hiding this comment.
THIS MESSAGE IS GENERATED BY AN AUTOMATED PROCESS.
Addressed this feedback and pushed commit e82c47c0.
Original feedback: #242 (comment)
Automated run notes:
Feedback Addressed
Moved the mov/lea comparison in challenges/computing-101/hello-hackers/open-read-write-rip-relative/DESCRIPTION.md until after lea is introduced.
Also updated .agents/skills/authoring-challenges/SKILL.md with a local-ordering rule so future DESCRIPTION edits do not use an instruction, term, or comparison before introducing it.
Removed the added envp[] pointer-table wording from challenges/computing-101/the-stack-revisited/gdb-stack-align-2/DESCRIPTION.md.
The file now matches origin/main for this item.
Removed the confusing envp[] pointer-table sentence from challenges/computing-101/the-stack-revisited/mem-stack-align/DESCRIPTION.md.
Kept the direct clarification that the environment string includes FOO=, the value, and the trailing null byte.
Restored all challenges/web-security/** files to origin/main, including deleting the PR-added challenges/web-security/common/Dockerfile.j2.
The effective diff against origin/main now contains no web-security changes.
Validation
git diff --check: clean.
nix develop --command pwnshop test challenges/computing-101/hello-hackers/open-read-write-rip-relative: passed, 1 challenge / 1 testcase.
No web-security tests were run because the requested resolution was to remove all web-security changes from the PR.
| @@ -1 +1 @@ | |||
| {% include "../../../common/Dockerfile.j2" %} | |||
There was a problem hiding this comment.
Remove all changes to web-security.
There was a problem hiding this comment.
THIS MESSAGE IS GENERATED BY AN AUTOMATED PROCESS.
Addressed this feedback and pushed commit e82c47c0.
Original feedback: #242 (comment)
Automated run notes:
Feedback Addressed
Moved the mov/lea comparison in challenges/computing-101/hello-hackers/open-read-write-rip-relative/DESCRIPTION.md until after lea is introduced.
Also updated .agents/skills/authoring-challenges/SKILL.md with a local-ordering rule so future DESCRIPTION edits do not use an instruction, term, or comparison before introducing it.
Removed the added envp[] pointer-table wording from challenges/computing-101/the-stack-revisited/gdb-stack-align-2/DESCRIPTION.md.
The file now matches origin/main for this item.
Removed the confusing envp[] pointer-table sentence from challenges/computing-101/the-stack-revisited/mem-stack-align/DESCRIPTION.md.
Kept the direct clarification that the environment string includes FOO=, the value, and the trailing null byte.
Restored all challenges/web-security/** files to origin/main, including deleting the PR-added challenges/web-security/common/Dockerfile.j2.
The effective diff against origin/main now contains no web-security changes.
Validation
git diff --check: clean.
nix develop --command pwnshop test challenges/computing-101/hello-hackers/open-read-write-rip-relative: passed, 1 challenge / 1 testcase.
No web-security tests were run because the requested resolution was to remove all web-security changes from the PR.
1 participant
THIS MESSAGE IS GENERATED BY AN AUTOMATED PROCESS.
Summary
challenges/computing-101/the-stack-revisited/mem-stack-align/DESCRIPTION.mdandgdb-stack-align-2/DESCRIPTION.md: learners are now told to count environment string bytes, includingNAME=, value, and the trailing null byte, rather than a separateenvp[]pointer slot.challenges/computing-101/hello-hackers/open-read-write-rip-relative/DESCRIPTION.mdto distinguish the address of"/flag\0"from the bytes stored there.challenges/computing-101/hello-hackers/open-read-write-rip-relative/challenge/.py/chal.pyso the checker acceptsxor reg, regas a valid zeroing idiom, matching a recorded solve.tee/ output-process-substitution nudge inchallenges/linux-luminarium/piping/procsub-write/DESCRIPTION.mdwithout giving the full solve command.challenges/web-security/DESCRIPTION.mdandpath-traversal-1/DESCRIPTION.md.challenges/web-security/common/Dockerfile.j2so terminal-only workflows havecurlavailable.challenges/web-security/xss-stored-alertto match its DESCRIPTION by using the existing Selenium alert checker, switching its Dockerfile to the Selenium base, and updating the private solve payload to triggeralert("PWNED").Validation
nix develop --command pwnshop test challenges/computing-101/hello-hackers/open-read-write-rip-relative: passed, 1 challenge / 1 testcase.open-read-write-rip-relativecast assembly throughnix develop --command pwnshop run --timeout 20 ...: passed and printed the flag.nix develop --command pwnshop test challenges/web-security/xss-stored-alert: passed, 1 challenge / 2 testcases.pwnshop-test-attempt-4.log: all tests passed, 21 challenges / 38 testcases.git diff --check: clean.Notes / deferred follow-ups