Fix Insecure Scrubs#104
Conversation
Add cxsecShred() to cxsec.c and cxsec.h. Clean up newline spacing in cxsec.c.
Update cxssShred() to use cxsecShred() instead of memset(). Improve the signature of cxssShred(). Improve the doc comment on cxssShred(). Clean up.
Remove #include "cxss/cxss.h". Add #include directives to include only the things that policy.h actually needs. Clean up.
Greptile SummaryThis PR replaces
Confidence Score: 5/5Safe to merge; all credential-zeroing paths are correctly hardened and the core shred implementation is sound. The volatile-write loop in cxsecShred() is the standard, correct approach for secure memory erasure before C23, and the previous reviewer concern about incomplete byte coverage has been addressed. All replaced memset calls are in paths that genuinely hold credentials, and the initialization memset in mtsession.c is correctly preserved. centrallix/include/cxss/policy.h — CXSS_IDENTIFIER_LENGTH is used but not defined by any of its included headers; works today but would break a direct include.
|
| Filename | Overview |
|---|---|
| centrallix-lib/src/cxsec.c | Adds cxsecShred() using a volatile uint8_t* loop — correctly prevents compiler elimination of zero-writes across all bytes; resolves the previous volatile-read-back concern. |
| centrallix-lib/include/cxsec.h | Adds declaration for cxsecShred(); straightforward header change. |
| centrallix-lib/src/mtsession.c | Replaces memset() on MtSession (which stores username/password) with cxsecShred() in all authentication error paths and session teardown; initialization memset retained correctly. |
| centrallix-lib/src/xringqueue.c | Replaces memset() with cxsecShred() on struct teardown and during queue reallocation pointer migration; not credential data per se, but shredding is not harmful. |
| centrallix/cxss/cxss_utility.c | Rewrites cxssShred() to delegate to cxsecShred() and updates the signature from int/unsigned char* to void/void*; correct and consistent. |
| centrallix/include/cxss/policy.h | Replaces circular #include "cxss/cxss.h" with targeted includes, but CXSS_IDENTIFIER_LENGTH (used in the struct) is no longer defined by any included header — it works only because cxss.h defines the macro before including this file. |
| centrallix/include/cxss/cxss.h | Updates cxssShred() signature from int/unsigned char* to void/void*; straightforward header fix. |
| centrallix/cxss/cxss_credentials_mgr.c | Adds cxss.h include and replaces memset on rand_key with cxssShred; correct. |
| centrallix/cxss/cxss_crypto.c | Adds cxss.h include and replaces memset in cxssDestroyKey with cxssShred before free(); correct. |
| centrallix/cxss/cxss_keystream.c | Replaces memset on CxssKeystreamState (holds cipher context) with cxssShred before nmFree; correct. |
| centrallix/netdrivers/net_http_sess.c | Replaces memset on NhtSessionData with cxssShred; cxssShred is reachable via net_http.h → cxss/cxss.h, so no missing include. |
| centrallix/osdrivers/objdrv_mysql.c | Replaces memset on conn->Password with cxssShred in connection eviction paths; correct and targeted. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["memset(ptr, 0, n)\n(may be optimised away)"] -->|"replaced by"| B
B["cxssShred(ptr, n)\ncentrallix/cxss/cxss_utility.c"]
B --> C["cxsecShred(ptr, n)\ncentrallix-lib/src/cxsec.c"]
C --> D["volatile uint8_t* loop\n\u2014 write cannot be eliminated"]
subgraph "Call sites replaced"
E["mtsession.c \u2014 MtSession (username/password)"]
F["cxss_credentials_mgr.c \u2014 rand_key"]
G["cxss_crypto.c \u2014 cxssDestroyKey"]
H["cxss_keystream.c \u2014 CxssKeystreamState"]
I["net_http_sess.c \u2014 NhtSessionData"]
J["objdrv_mysql.c \u2014 conn->Password"]
K["xringqueue.c \u2014 XRingQueue struct & pointer array"]
end
E & F & G & H & I & J & K --> B
Reviews (3): Last reviewed commit: "Replace shredding method that AI thought..." | Re-trigger Greptile
|
This PR is ready for human review. |
…thod that is ACTUALLY secure (I think).
Several locations in centrallix and centrallix-lib scrub credentials using
memset(), which may be optimized away.This PR implements
cxsecShred()in centrallix-lib, which does a volatile read after usingmemset()so that it cannot be optimized away (needed becausememset_s()andmemset_explicit()are not available). The PR replacesmemset()calls to shred sensitive data in centrallix-lib withcxsecShred(). It also replaces thememset()call incxssShred(), and uses that function to shred data in centrallix, replacingmemset()calls there.I did a full review of all 500+
memset()calls in the codebase and I believe that none of the remaining calls are used to shred sensitive data (other than the one call incxsecShred()described above, obviously). I also audited every line changed to a shred call, and every call shreds data that might contain credentials, so I don't think any of them are unnecessary.I'm planning to make another PR that adds the autoconf macros required to support other shredding solutions in centrallix-lib, and I'll link that here when it is done.