Bug 7253 - Samba login-cache problem on Sparc Architecture
Samba login-cache problem on Sparc Architecture
Status: RESOLVED FIXED
Product: Samba 3.5
Classification: Unclassified
Component: User & Group Accounts
3.5.0
Sparc Solaris
: P3 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
http://bugs.opensolaris.org/bugdataba...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-15 14:20 UTC by Vladimír Marek
Modified: 2010-06-09 08:44 UTC (History)
1 user (show)

See Also:


Attachments
Proposed patch (1.55 KB, patch)
2010-03-16 15:11 UTC, Volker Lendecke
obnox: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Vladimír Marek 2010-03-15 14:20:21 UTC
I'll just copy the original description which is attached to our internal CR (change request).

The Customer configures the "bad password tries before lockout" to 3:

/usr/sfw/bin/pdbedit -P "bad lockout attempt" -C 3 After that the customer
gets the correct behaviour on Solaris x86 Intel:

a user will "autolocked" after the 3rd "bad password try".

In opposite, on Solaris Sparc it does not work: the user gets "autolocked"
already after the 1st "bad password try".

o Reason:

There is a "wrong datatype access" <-> "architecture memory storage"
(little/big endian) problem in samba source-code !


o Detailed Analyses:

log-file of smb-login-cache:

-> GOOD-case: (on Intel)

[2009/11/26 14:27:41, 5] passdb/login_cache.c:(111)
  Found login cache entry: timestamp 1259242056, flags 0x69740010, count 1, time 1259242056
                                                         ^^^^^^^^
                                                              |<--- 0x0 (3rd digit) means: login-try still allowed
[2009/11/26 14:27:41, 7] passdb/pdb_ldap.c:(911)
  ldap time is 1259242006, cache time is 1259242056, bad time = 1259242056
[2009/11/26 14:28:14, 5] passdb/login_cache.c:(111)
  Found login cache entry: timestamp 1259242061, flags 0x69740010, count 2, time 1259242061
[2009/11/26 14:28:14, 7] passdb/pdb_ldap.c:(911)
  ldap time is 1259242006, cache time is 1259242061, bad time = 1259242061
[2009/11/26 14:44:23, 5] passdb/login_cache.c:(111)
  Found login cache entry: timestamp 1259249461, flags 0x69740410, count 3, time 1259249461
                                                         ^^^^^^^^
                                                              |<--- 0x4 (3rd digit) means: login locked
[2009/11/26 14:44:23, 7] passdb/pdb_ldap.c:(911)
  ldap time is 1259249462, cache time is 1259249461, bad time = 1259249461


-> BAD-case: (on Sparc)

[2009/11/25 13:12:42, 5] passdb/login_cache.c:(111)
  Found login cache entry: timestamp 1259154723, flags 0x107469, count 1, time 1259154723
                                                         ^^^^^^
                                                            |<--- 0x4 (3rd digit) means: login locked
[2009/11/25 13:12:42, 7] passdb/pdb_ldap.c:(911)
  ldap time is 1259154696, cache time is 1259154723, bad time = 1259154723


--> on Sparc there is the 0x4 ({"ACB_AUTOLOCK", 0x0400}) already at the 1st try "count 1"


==> note the summary:

    good:
      Found login cache entry: timestamp 1259242056, flags 0x69740010, count 1, time 1259242056
    bad:
      Found login cache entry: timestamp 1259154723, flags 0x107469, count 1, time 1259154723
      --> with leading "0":
      Found login cache entry: timestamp 1259154723, flags 0x00107469, count 1, time 1259154723

        GOOD-case Intel:
            flags 0x69740010, count 1       ...user not locked yet
            flags 0x69740410, count 3       ...user locked at 3rd try
                        ^^^^
                      acct_ctrl, see below

        BAD-Fall Sparc:
            flags 0x00107469, count 1       ...user locked at 1st try
                    ^^^^
                  acct_ctrl, see below

==> it seems:
    that the 32bit "flags" consists of two 16bit short values (acct_ctrl 0x0010 and "any other" 0x6974).
    and this two 16bit values are swapped (hi/lo) within the 32bit memory variable !
    (additionally the 2 bytes "other" 0x6974 are swapped a 2nd time: 0x7469)

    --> this leads to assumption:
        it has something to do with little/big-endian storage of these 16 bit short values
        within 32bit oriented memory.


==> caused by the fortuity that the "any other" 16bit value contains a 0x4 on 3rd digit, the customer
    gets the effect, that the user will be autolocked already after the 1st "bad password try".


o Cause of problem:

--> if you look into the source code, the cause of this failure will be obviously:

samba-3.0.28/passdb/login_cache.c

#define SAM_CACHE_FORMAT "dwwd"             <========

LOGIN_CACHE * login_cache_read(struct samu *sampass) {
    TDB_DATA keybuf, databuf;
    LOGIN_CACHE *entry;
...
    if (tdb_unpack (databuf.dptr, databuf.dsize, SAM_CACHE_FORMAT,
            &entry->entry_timestamp, &entry->acct_ctrl,
            &entry->bad_password_count,
            &entry->bad_password_time) == -1) {
        DEBUG(7, ("No cache entry found\n"));
        SAFE_FREE(entry);
        SAFE_FREE(databuf.dptr);
        return NULL;
    }
...
}

BOOL login_cache_write(const struct samu *sampass, LOGIN_CACHE entry) { ...
    databuf.dsize =
        tdb_pack(NULL, 0, SAM_CACHE_FORMAT,
             entry.entry_timestamp,
             entry.acct_ctrl,
             entry.bad_password_count,
             entry.bad_password_time);
    databuf.dptr = SMB_MALLOC_ARRAY(char, databuf.dsize); ...
}

==> the value "flags" in the smb-cache entry represents: LOGIN_CACHE *entry->acct_ctrl
    ...and will be stored/read by tdb_pack()/tdb_unpack() functions.

samba-3.0.28/include/passdb.h

/* cache for bad password lockout data, to be used on replicated SAMs */ typedef struct logon_cache_struct {
    time_t entry_timestamp;
    uint32 acct_ctrl;                       <==== here flags/acct_ctrl is defined as uint32 !!!
    uint16 bad_password_count;
    time_t bad_password_time;
} LOGIN_CACHE;


==> in the definition of such a cache-entry the variable "acct_ctrl" is 32bit.
    ...therefore the log-file shows also 32bit "flags".
    BUT:
    the pack/unpack functions work with a 16bit value to store/read into/from the cache !!!

        #define SAM_CACHE_FORMAT "dwwd"
                                  ^^^^^<========

samba-3.0.28/lib/util_tdb.c

/****************************************************************************
 Useful pair of routines for packing/unpacking data consisting of  integers and strings.
****************************************************************************/
size_t tdb_pack_va(char *buf, int bufsize, const char *fmt, va_list ap) {
    uint8 bt;
    uint16 w;
    uint32 d;
...
    while (*fmt) {
        switch ((c = *fmt++)) {
...
        case 'w': /* unsigned 16-bit integer */
            len = 2;
            w = (uint16)va_arg(ap, int);                    <============= format "w" means uint16 !!!
            if (bufsize && bufsize >= len)
                SSVAL(buf, 0, w);
            break;
        case 'd': /* signed 32-bit integer (standard int in most systems) */
            len = 4;
            d = va_arg(ap, uint32);
            if (bufsize && bufsize >= len)
                SIVAL(buf, 0, d);
            break;
...
}


==> this is the root-cause of the problem:
    a 16bit value will be stored into a 32bit memory variable,
    ...which is not a problem on Intel-architecture, but on Sparc:

  Intel:

    mem-addr
        |
        l  L  h  H     <-- memory storage
        l  L  ?  ?
        |
       write 16bit to addr

  Sparc:

    mem-addr
        |
        H  h  L  l     <-- memory storage
        L  l  ?  ?
        |
       write 16bit to addr


o Solution:

Storing a 32bit value into a 32bit variable should solve the problem, because it is architecture independent.
--> I suggested to the customer:
    - change of the pack/unpack-format in source-code
    - and re-compile the sampa-package

samba-3.0.28/passdb/login_cache.c
    old, wrong:
            #define SAM_CACHE_FORMAT "dwwd"
    change to
    new, correct:
            #define SAM_CACHE_FORMAT "ddwd"

the customer has agreed, recompiled samba and tried it in his environment, and can
confirm: This fix helps to solve the problem !


==> BUT: the problem occurs with the official (by Sun) delivered package too !
    so this is the reason for this Bug-report.
Comment 1 Volker Lendecke 2010-03-16 15:11:21 UTC
Created attachment 5505 [details]
Proposed patch

Thanks for the detailed report.

I would rather go with a slightly different patch, see the attachment. While it seems perfectly fine to change the login_cache.tdb data format on upgrade (we detect failure due to too small entries), downgrade to a faulty version would render invalid data in login_cache_read. So I would prefer the attached patch that does not alter the on-disk data format.

It is also safe because as far as I can see we only use the AUTOLOCK bit from that field which fits into the lower 16 bit.

Please test the attached patch before I push it.

Thanks,

Volker
Comment 2 Karolin Seeger 2010-05-12 03:55:32 UTC
Any feedback on the proposed patch?
Comment 3 Guenther Deschner 2010-05-27 05:12:33 UTC
Seems like adressed in a slightly different manner, Jeremy / Volker can you please check and close if appropriate ?
Comment 4 Vladimír Marek 2010-06-07 06:49:36 UTC
(In reply to comment #2)
> Any feedback on the proposed patch?
> 

Hi,

I'm sorry for the late reply, customer replied just now. The fix suggested by you works. We are going to include into our distribution.

Thank you a lot

-- 
  Vlad
Comment 5 Michael Adam 2010-06-09 04:47:49 UTC
Comment on attachment 5505 [details]
Proposed patch

nice fix for the problem!
Comment 6 Michael Adam 2010-06-09 04:48:24 UTC
Assigning to Karolin for picking into the next release.
Comment 7 Karolin Seeger 2010-06-09 08:44:58 UTC
Pushed to v3-5-test.
Will be included in 3.5.4.

Closing out bug report.

Thanks!