[PATCH v3 0/6] samba: fix conversion of strings to binary SIDs and vice versa

Jeff Layton jlayton at samba.org
Wed Jul 31 08:38:17 MDT 2013


This is the third posting of this set. Summary of changes:

v3:
- add torture tests
- fix bad mask in dom_sid_parse_endp
- eliminate extra '-' in sid-to-string conversions

v2:
- fix conversion logic in sid-to-string routines
- fix the libcli versions of these routines
- eliminate unneeded "code cleanup" deltas and casts
- ensure that we add ULL specifiers on long constants
- use UINT32_MAX instead of the more ambiguous UINT_MAX

The original patch description follows:

While implementing a similar routine for cifs-utils, I used the logic in
wbcStringToSid. Unfortunately, I couldn't use it directly due to a
number of problems in this code (some worse than others):

- We're not converting signed integers here, so it makes more sense to
  to use strtou* variants.

- The code doesn't check whether the revision string is larger than a
  uint8_t can hold. It should.

- It's possible to have legitimate authority bytes in fields 0 and 1,
  so we need to convert that value using strtoull and mask and shift
  those bytes too.

- We should also ensure that the value in the authority field does not
  exceed what the id_auth array can hold. We should also check for it
  to return ULONG_MAX. Checking the returned value vs. a newly declared
  AUTHORITY_MASK should cover both cases.

- If the authority is >= UINT_MAX then it should be represented as a
  hex value (according to MS-DTYP).

- rather than just ignoring high-order bits when converting the
  subauthority values, we should instead ensure that it's not
  larger than a 32-bit value, and throw an error if it is.

- the check for a NULL endptr value in the subauthority loop seems
  to be entirely bogus. It's not clear to me when strtoul would ever
  set that to NULL.

Now, all that said...I'm not very familiar with this code and I don't
have a great way to test it so I'd prefer someone with more knowledge
of this code take these patches and shepherd them in.

Thanks!

Jeff Layton (6):
  wbclient: fix conversion logic in wbcStringToSid
  wbclient: fix conversion logic in wbcSidToStringBuf
  libcli: fix conversion logic in dom_sid_parse_endp
  libcli: fix conversion logic in dom_sid_string_buf
  torture: add more string_to_sid torture testcases
  torture: add LOCAL-sid_to_string testcase

 libcli/security/dom_sid.c      | 57 ++++++++++++++++++++---------------
 nsswitch/libwbclient/wbc_sid.c | 68 +++++++++++++++++++++++-------------------
 source3/selftest/tests.py      |  1 +
 source3/torture/torture.c      | 46 ++++++++++++++++++++++++++++
 4 files changed, 117 insertions(+), 55 deletions(-)

-- 
1.8.3.1



More information about the samba-technical mailing list