[PATCH RFC] libwbclient: fix conversion logic in wbcStringToSid
Volker Lendecke
Volker.Lendecke at SerNet.DE
Sun Jul 21 08:22:11 MDT 2013
Hi, Jeff!
This looks very good, thanks!
We have another function called dom_sid_parse_endp somewhere
that might need similar care...
On Fri, Jul 12, 2013 at 04:24:12PM -0400, Jeff Layton wrote:
> From: Jeff Layton <jlayton at samba.org>
>
> 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 with this code:
>
> - 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 consider this a RFC patch...
>
> Signed-off-by: Jeff Layton <jlayton at samba.org>
> ---
> nsswitch/libwbclient/wbc_sid.c | 41 ++++++++++++++++++++++-------------------
> 1 file changed, 22 insertions(+), 19 deletions(-)
>
> diff --git a/nsswitch/libwbclient/wbc_sid.c b/nsswitch/libwbclient/wbc_sid.c
> index bab6933..f27adf9 100644
> --- a/nsswitch/libwbclient/wbc_sid.c
> +++ b/nsswitch/libwbclient/wbc_sid.c
> @@ -88,13 +88,15 @@ wbcErr wbcSidToString(const struct wbcDomainSid *sid,
> return WBC_ERR_SUCCESS;
> }
>
> +#define AUTHORITY_MASK (~(0xffffffffffffULL))
> +
> /* Convert a character string to a binary SID */
> wbcErr wbcStringToSid(const char *str,
> struct wbcDomainSid *sid)
> {
> const char *p;
> char *q;
> - uint32_t x;
> + uint64_t x;
> wbcErr wbc_status = WBC_ERR_UNKNOWN_FAILURE;
>
> if (!sid) {
> @@ -115,46 +117,47 @@ wbcErr wbcStringToSid(const char *str,
> /* Get the SID revision number */
>
> p = str+2;
> - x = (uint32_t)strtol(p, &q, 10);
> - if (x==0 || !q || *q!='-') {
> + x = (uint64_t)strtoul(p, &q, 10);
> + if (x==0 || x > UCHAR_MAX || !q || *q!='-') {
> wbc_status = WBC_ERR_INVALID_SID;
> BAIL_ON_WBC_ERROR(wbc_status);
> }
> sid->sid_rev_num = (uint8_t)x;
>
> - /* Next the Identifier Authority. This is stored in big-endian
> - in a 6 byte array. */
> -
> + /*
> + * Next the Identifier Authority. This is stored big-endian in a
> + * 6 byte array. If the authority value is > UINT_MAX, then it should
> + * be expressed as a hex value, according to MS-DTYP.
> + */
> p = q+1;
> - x = (uint32_t)strtol(p, &q, 10);
> - if (!q || *q!='-') {
> + x = (uint64_t)strtoull(p, &q, 0);
> + if (!q || *q!='-' || (x & AUTHORITY_MASK)) {
> wbc_status = WBC_ERR_INVALID_SID;
> BAIL_ON_WBC_ERROR(wbc_status);
> }
> - sid->id_auth[5] = (x & 0x000000ff);
> - sid->id_auth[4] = (x & 0x0000ff00) >> 8;
> - sid->id_auth[3] = (x & 0x00ff0000) >> 16;
> - sid->id_auth[2] = (x & 0xff000000) >> 24;
> - sid->id_auth[1] = 0;
> - sid->id_auth[0] = 0;
> + sid->id_auth[5] = (x & 0x0000000000ff);
> + sid->id_auth[4] = (x & 0x00000000ff00) >> 8;
> + sid->id_auth[3] = (x & 0x000000ff0000) >> 16;
> + sid->id_auth[2] = (x & 0x0000ff000000) >> 24;
> + sid->id_auth[1] = (x & 0x00ff00000000) >> 32;
> + sid->id_auth[0] = (x & 0xff0000000000) >> 48;
I'm not familiar with C enough (after having tried to
program it for 2 decades...) -- 0xff0000000000, doesn't that
possibly need a LL specifier at the end on 32 bit?
>
> /* now read the the subauthorities */
> -
> p = q +1;
> sid->num_auths = 0;
> while (sid->num_auths < WBC_MAXSUBAUTHS) {
> - x=(uint32_t)strtoul(p, &q, 10);
> + x=(uint64_t)strtoull(p, &q, 10);
Is this cast really necessary? Similarly above.
> if (p == q)
> break;
> - if (q == NULL) {
> + if (x > UINT_MAX) {
UINT_MAX, can't that be 2^64-1 on 64 bit? Shouldn't we use
UINT32_MAX here?
> wbc_status = WBC_ERR_INVALID_SID;
> BAIL_ON_WBC_ERROR(wbc_status);
> }
> sid->sub_auths[sid->num_auths++] = x;
>
> - if (*q != '-') {
> + if (*q != '-')
> break;
> - }
> +
That's Samba style, Linux kernel is different...
Thanks,
Volker
--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
More information about the samba-technical
mailing list