Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
lib/openldap.c can leak allocated LDAP values when rejecting a malformed attribute type.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Hardens LDAP LDIF output against injection from unsafe DNs and attribute names.
Changes:
- Base64-encodes non-SAFE DNs.
- Rejects attribute type names containing control bytes.
- Reuses SAFE-STRING validation for values.
File summaries
| File | Summary |
|---|---|
lib/openldap.c |
Adds DN encoding and attribute-type validation. |
lib/ldap.c |
Adds equivalent handling for the standard LDAP backend. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if(oldap_str_needs_base64(bv.bv_val, bv.bv_len)) { | ||
| result = CURLE_WEIRD_SERVER_REPLY; | ||
| break; | ||
| } |
|
copilot was right about the bvals leak, when the rejected type had values the early break skipped the ber_memfree. valgrind showed 48 bytes lost from ldap_get_attribute_ber on the old version, clean now. also added a failf so the user sees why it stopped instead of a bare (8). re-ran the mock server cases: LF in the DN comes out as one DN:: base64 line, LF in a type name errors out with the message, clean and utf-8 entries unchanged. |
c4473d0 to
1eee9cb
Compare
The LDIF output already base64-encodes attribute values that are not RFC 2849 SAFE-STRINGs, but the entry DN and the attribute type names were still written verbatim. A hostile or MITM LDAP server that returns a DN or a type name containing CR/LF could therefore inject its own lines into the LDIF that curl prints. Follow RFC 2849 for those two fields as well: write a non-SAFE DN in the "DN:: <base64>" form, and reject a type name that carries a control byte (a type has no base64 form in LDIF and a conformant one never contains one). The SAFE-STRING test is factored into one helper per backend and the existing value check now shares it. Applies to both the OpenLDAP (lib/openldap.c) and the generic/WinLDAP (lib/ldap.c) backends.
1eee9cb to
2960c68
Compare
|
Thanks! |
Follow-up to the value base64 work (2ba2fe3 / #22339). Values that aren't
SAFE-STRINGs get base64-encoded now, but the entry DN and the attribute type
names are still written to the LDIF verbatim. A hostile or MITM LDAP server that
puts a CR/LF in a returned DN or type name can inject its own lines into the
LDIF curl prints, and if that output is later fed to an LDIF processor those
lines look like real directives.
RFC 2849 says a DN that isn't a SAFE-STRING has to use the
dn::base64 form,and a valid attribute description can't hold a control byte at all. So this does
the same for those two fields: write a non-SAFE DN as
DN:: <base64>, and rejecta type name carrying a control byte (a type has no base64 form in LDIF, and a
conformant one never has one). The SAFE-STRING test is pulled into one helper per
backend and the existing value check reuses it. Done for both
lib/openldap.cand
lib/ldap.c.Tested on the OpenLDAP backend with a small mock server: a DN with an embedded
LF now comes out as a single
DN::base64 line that decodes back to the original(no injected line), a type name with an LF is rejected with
CURLE_WEIRD_SERVER_REPLY, and a normal DN and a base64'd value are unchanged.Build and checksrc are clean. curl has no LDAP test server in the suite so this
path isn't covered by runtests, same as the value fix was.
Reported via HackerOne (#4023506).