Add NA values for NX and fortify_source#161
Conversation
ffontaine
force-pushed
the
add-na-values
branch
from
1861f4e to
20e5442
Compare
There was a problem hiding this comment.
Pull request overview
This PR adjusts checksec reporting to support an explicit “NA” (not applicable) state for NX (e.g., kernel modules / platforms without NX) and for fortify_source when no fortifiable functions are detected (e.g., binaries built with toolchains without fortify support).
Changes:
- Introduces
NXTypeand updates NX detection to returnYes/No/NAinstead of a boolean. - Introduces
FortifyTypeand updates ELF fortify-source detection to returnYes/No/NAinstead of a boolean. - Updates Rich and JSON output formatting to display/emit the enum names.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
checksec/output.py |
Updates Rich/JSON rendering to handle and display NXType/FortifyType values (including NA). |
checksec/elf.py |
Adds FortifyType and changes fortify-source computation to return Yes/No/NA. |
checksec/binary.py |
Adds NXType and changes NX detection to return Yes/No/NA (including NA for ELF with no segments). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| else: | ||
| nx_res = "[green]Yes" | ||
| nx_res = f"[green]{nx.name}" | ||
| row_res.append(nx_res) |
There was a problem hiding this comment.
In the PE output branch, row_res is not defined, but row_res.append(nx_res) is called. This will raise a NameError at runtime when processing PE files. Remove the row_res usage here (since table_pe.add_row(...) already uses nx_res directly), or initialize a local list before appending.
| row_res.append(nx_res) |
| def add_checksec_result(self, filepath: Path, checksec: Union[ELFChecksecData, PEChecksecData]): | ||
| if isinstance(checksec, ELFChecksecData): | ||
| self.data[str(filepath.resolve())] = { | ||
| "relro": checksec.relro.name, | ||
| "canary": checksec.canary, | ||
| "nx": checksec.nx, | ||
| "nx": checksec.nx.name, | ||
| "pie": checksec.pie.name, | ||
| "rpath": checksec.rpath, | ||
| "runpath": checksec.runpath, | ||
| "symbols": checksec.symbols, | ||
| "fortify_source": checksec.fortify_source, | ||
| "fortify_source": checksec.fortify_source.name, | ||
| "fortified": checksec.fortified, | ||
| "fortify-able": checksec.fortifiable, | ||
| "fortify_score": checksec.fortify_score, | ||
| } |
There was a problem hiding this comment.
JSONOutput.add_checksec_result() always does checksec.fortify_source.name, but ELFSecurity.checksec_state sets fortify_source=None when no libc is detected. Running checksec -j without a libc will therefore crash with an AttributeError. Gate fortify fields behind self._libc_detected (like RichOutput does) or handle None explicitly (e.g., emit null / omit the key).
| self.data[str(filepath.resolve())] = { | ||
| "relro": checksec.relro.name, | ||
| "canary": checksec.canary, | ||
| "nx": checksec.nx, | ||
| "nx": checksec.nx.name, | ||
| "pie": checksec.pie.name, | ||
| "rpath": checksec.rpath, | ||
| "runpath": checksec.runpath, | ||
| "symbols": checksec.symbols, | ||
| "fortify_source": checksec.fortify_source, | ||
| "fortify_source": checksec.fortify_source.name, | ||
| "fortified": checksec.fortified, | ||
| "fortify-able": checksec.fortifiable, | ||
| "fortify_score": checksec.fortify_score, | ||
| } | ||
| elif isinstance(checksec, PEChecksecData): | ||
| self.data[str(filepath.resolve())] = { | ||
| "nx": checksec.nx, | ||
| "nx": checksec.nx.name, | ||
| "canary": checksec.canary, |
There was a problem hiding this comment.
This change switches JSON output for nx (and fortify_source) from booleans to enum names ("Yes"/"No"/"NA"). The repository’s E2E tests currently assert these JSON fields are booleans (see tests/e2e/test_e2e_elf.py::test_bool_prop and tests/e2e/test_e2e_pe.py::test_bool_prop), so this will break the existing contract. Either keep booleans and represent “not applicable” as null (or a separate field), or update the tests (and any documented schema expectations) to match the new string values.
| @property | ||
| def has_nx(self) -> bool: | ||
| # Handle ELF binary with no program segments (e.g., Kernel modules) | ||
| # In this case, return True | ||
| if isinstance(self.bin, lief.ELF.Binary) and len(self.bin.segments) == 0: | ||
| return True | ||
| return NXType.NA | ||
| elif self.bin.has_nx: | ||
| return NXType.Yes | ||
| else: | ||
| return self.bin.has_nx | ||
| return NXType.No |
There was a problem hiding this comment.
has_nx is still annotated as returning bool, but it now returns NXType values. This type mismatch will mislead readers and static type checkers; update the return annotation (and any related docstrings/type hints) to NXType.
ffontaine
force-pushed
the
add-na-values
branch
8 times, most recently
from
90c7833 to
fc79f9b
Compare
Return NA instead of True for kernel module and NA instead of True when 0 fortifiable functions were detected in the binary. This will avoid to return incorrect values for binaries compiled with a toolchain that does not support fortify_source (e.g. musl) on a processor that does not support NX (e.g. mips) Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
ffontaine
force-pushed
the
add-na-values
branch
from
fc79f9b to
7773468
Compare
Return NA instead of True for kernel module and NA instead of True when 0 fortifiable functions were detected in the binary. This will avoid to return incorrect values for binaries compiled with a toolchain that does not support fortify_source (e.g. musl) on a processor that does not support NX (e.g. mips)