[PATCH RFC] libwbclient: fix conversion logic in wbcStringToSid
Jeff Layton
jlayton at redhat.com
Mon Jul 22 04:49:55 MDT 2013
On Sun, 21 Jul 2013 16:22:11 +0200
Volker Lendecke <Volker.Lendecke at SerNet.DE> wrote:
> Hi, Jeff!
>
> This looks very good, thanks!
>
> We have another function called dom_sid_parse_endp somewhere
> that might need similar care...
>
Ok, I'll take a look at that too.
> 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?
>
It sure does -- sparse complains about that too. I fixed it in the
cifs-utils version of this patch, I'll go ahead and fix it too. Good
catch.
> >
> > /* 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.
>
Probably not, at least on most arches. strtoull returns an unsigned
long long, so if you assume that that's the same as a uint64_t on all
arches then it's not really necessary.
> > 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?
>
As far as I know, all arches (at least on linux) make sure that "int"
is 32 bit. That said, UINT32_MAX does sound more explicit. I'll fix it.
> > 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...
>
Fair enough -- I'll drop that hunk.
Thanks for the review. I'll send a v2 patch when I get a chance.
--
Jeff Layton <jlayton at redhat.com>
More information about the samba-technical
mailing list