Will have a look.

> find . -name "*.c" -exec grep --with-filename strcpy {} \; | wc -l
417
> find . -name "*.c" -exec grep --with-filename strcat {} \; | wc -l
62
> find . -name "*.c" -exec grep --with-filename sprintf {} \; | wc -l
254
> find . -name "*.c" -exec grep --with-filename "/tmp" {} \; | wc -l
9
> find . -name "*.c" -exec grep --with-filename -E "system\(.*\)" {} \;  | wc -l
29


Too much usage of high-profile (security-wise) functions.

The above is the novell-lum code.

./config/confuncs.c:    ret = system("sed /pam_ndssso/d /etc/pam.conf | sed s/pam_nds/pam_nam/ >/tmp/pam.conf.up; cp -f /tmp/pam.conf.up /etc/pam.conf");

./config/confuncs.c:    //ret = system("cd /etc/pam.d; for pfile in `ls /etc/pam.d`; do sed /pam_ndssso/d $pfile| sed s/pam_nds/pam_nam/ > /tmp/pam.up; cp -f /tmp/pam.up /etc/pam.d/$pfile ; done; cd -");

./config/confuncs.c:    ret = system("cd /etc/pam.d ; for pfile in `ls /etc/pam.d`; do if [ -f $pfile ]; then sed /pam_ndssso/d $pfile| sed s/pam_nds/pam_nam/ > /tmp/pam.up; cp -f /tmp/pam.up /etc/pam.d/$pfile; fi; done ; cd -");

./config/confuncs.c:    ret = system("sed s/nds/nam/ /etc/nsswitch.conf > /tmp/nsswitch.conf.up; cp -f /tmp/nsswitch.conf.up /etc/nsswitch.conf");

Not even bad programming stlye but also a security risc.

Can you please elaborate on comment #3 ?
The code you mentioned in in the comment #4 will never be executed in Code 10.
This code provides upgrade from old Novell Account Manager product to LUM.

adding myself to CC list

Also keep in mind that we are not distributing the source code for novell-lum yet.

comment #5:

- I assume you mean comment #2. The functions very likely lead to security problems like using strcpy(namConfigFile, optarg) will overflow the stack/heap and can be used to execute code. I did not check for exploitability because there are just too much of them. The counting of these functions is just to get a quick overview of the code. Please use saver replacements that do check for the size of the destination buffer (strncpy(), strncat(), snprintf()), do NOT use system() or pipe(), and do NOT write
to /tmp blindly

- If the code will never be used then remove it please. It's a potential security risk.

The usage of system() to copy, remove, or rename files/dirs can be found in various code places. This is prone to security problems. Please look at other tools like pwdutils to see how to avoid calling system() and get the same tasks done faster and more secure.

Note: I will add my findings subsequently to this bug entry and not write them up in a report to get this bugs handled faster.

CHECK_LENGTH(len, size)
len is of type size_t
size is of type int (unsigned?)

On 64bit machines size_t is 8 byte and int is still 4 byte. This should be kept in mind to make SIZE256 (and a like) of tye (size_t) to improve portability between different architectures.

#define SIZE256 ((size_t)  256)

Comparing and assigning different integer types can lead to security problems... but a quick check did not reveal a problem for this case.

Ok, I already started replacing strcpy, strcat, sprintf with strncpy, strncat, snprintf. The system() calls you are reffering to can be invoked only with 
two conditions:

1. The older version of this product has to be installed on the system, which will never be Code 10 based :-)
2. User has to provide Administrative password for Administrator in the LDAP tree.
The last one means that this person has to be member of IS&T.

Also the source code you are looking also includes the set of test utilities. Executables for which we do not ship with runtime RPM.

Since there are a lot of changes need to be done with replacing strcpy, strcat, sprintf with strncpy, strncat, snprintf this may effect stability of code.

1. Are you OK if I checkin these changes post Code 10 ? 
2. If not, I would like to make these changes first in daemon and library code since they are more volnurable and if time permit make this changes 
in the utilities  ? Is it OK with you guys ?

Thanks.

Please keep in mind that strn*() functions do not necessarily null-terminate strings.

- wrt. 1.
So, what is the code used for? On which plattform?

- wrt. 2.
It doesn't matter. Using system() spawns a shell and this opens a bunch of security
problems (Like not using full pathnames for tools, writing to /tmp (i.e. /tmp/unix2edir_tmp2),
unexpected changing of the environment of operation, unknown tool options (like using
a path "-rf .." when calling rm and so on)).

And again it is a big overhead.

For example:
sprintf( sysbuffer, "%s %s %s", "ps auxw | grep ", dmn, " | grep -q -v grep");

Which is: ps auxw | grep dmn | grep -q -v grep

This means 4 extra processes have to be forked. (I known it will not be used under Linux, but it is a good example.)

C code
 |
+---- /bin/sh (1)
        |   |   |
        |   |   +----- grep (4)
     ps   grep (3)
     (2) 

All these shell tasks could be done cleaner, more efficient, and more secure in C code.

I assume this code is for testing if dmn runs. Use a PID file and then call kill(pid, 0) and you are done. This means three syscalls (open, read, kill) vs. 4 processes. :-)

Q1: I am not in the position to make the final decission what has to be in CODE10 and what has not to be there. But personally I would say: No, not with these bugs. (note: I just started looking at the code...)

Q2: That's perfectly ok with me. Thanks!

corerction:
"This means 4 extra processes have to be forked. (I known it will not be used
under Linux, but it is a good example.)"

This example is executed.

pam_sm_authenticate() function:
- char *userFDN,pass1[MB_LEN_MAX * PASS_LEN]
- if (_nds_pam_read_options(argc,(const char **)argv,pass,OPTIONS) == PAM_SUCCESS)
  {
	strcpy(pass1,pass);
  }

I can't test it but it seems to be a remote root bug.

pam_sm_authenticate():
- storeresult is not free'ed after a successful pam_set_data()

err... or will it be free'ed during cleanup?

common_passwd():
- 		if (userfdnret=_nds_getuserFDN(username,tmp,sizeof(tmp), NULL)!=PAM_SUCCESS)
		{
			if (userfdnret==PAM_USER_UNKNOWN)
			{
				sprintf(errmsg,UAM_QUOT_SQUOT_DOES_NOT_EXIST_OR_
                                QUOT_SQUOT_BELONGS_TO_A_GROUP_
                                WHICH_DOES_NOT_HAVE_ACCESS_RIGHTS_
                                TO_THE_CURRENT_WORKSTATION__OR_
                                UNABLE_TO_GET_QUOT_SQUOT_CONTEXT_
                                ,username,username,username);

This one looks like another root hole (local).

Can you provide more details on the comment #19 ? is it just sprintf you are reffering or more ? Thanks.

Just the sprintf() and the username (which can be considered as data from an untrusted source). Does everything fit into errmsg?

pam_misc.c:init_sec_salt()
- The salt is constant. A constant salt does not have an effect (= making a dictionary attack inpractical).

http://en.wikipedia.org/wiki/Password_salting

Use a changing value which is not related to the password (like a hash-value of the password) as salt. Something like "srand(time(NULL) ^ getpid()); rand()" should be ok for this task.

pam_misc.c:cert_callback()
I see the code for parsing and adding a certificate but where is the cert verified to be genuine?
Quickliy looking into the source-code of the novell-ldapext packages does not reveal anything.

additionally can cert.ength be trusted here "cert.data = (void *)malloc(cert.length); "?

pam_misc.c:itoa()
- just a note: on the borders of an integer this function seems not to work correctly

> ./itoa-test -123457777777777777
integer:  -2147483648
        n = -2147483648
        n % 10 = -8 + 48 ('0')
        s = (-
        s[i] = ( <-> s[j] = -
ascii: -(

n % 10 + '0' -->  -8 + 48 = 40 --> '('

Can you give me more details about the background of the following statement from the man-page of pan_nam, please?

"The server certificate signed by a Certificate Authority is created and stored in the local work
station during Novell Account Management (NAM) installation."

Does this mean the SSL server side does request a cert from the SSL client side, or do you want to verify the cert of the server?

pam_misc.c:CryptPasswordForCache():
- This function also includes a constant salt.

create_pam.c:mkdirp():
- strcpy() may overflow path
- the removing of trailing spaces doe not work, p_ptr already point to the end
  of the buffer which is 0
- strcat(path,"/"), will write a 0 behind the buffer if path is sizeof(path) bytes long
- check for NULL path is not needed b/c you already added a trailing '/'
- note: removing spaces from the beginning of the path can turn a relative path
  into an absolute path: "    /foo/" --> "/foo/"
  or: "        " will become "/"
- before the main while-loop you do a p_ptr++, if path is "/" this will point behind
  the buffer as far as I can see
- the "p_ptr = tmp_ptr + 1;" in the while-loop can point behind the buffer
Example:
> ./mkdirp-test "        /        "
MAIN: path is '        /        '
mkdirp:17: strcpy('        /        ')
mkdirp:20: set p_ptr to 3219801508 + 17 = 3219801525
mkdirp:22: removing trailing spaces in '        /        '
mkdirp:31: p_ptr = 3219801524, path = 3219801508
mkdirp:33: check for trailing '/' in '        /        '
mkdirp:37: added trailing '/': '        /        /'
mkdirp:40: p_ptr = path
mkdirp:43: removing leading spaces in '        /        /'
mkdirp:47: increment p_ptr to 3219801509
mkdirp:47: increment p_ptr to 3219801510
mkdirp:47: increment p_ptr to 3219801511
mkdirp:47: increment p_ptr to 3219801512
mkdirp:47: increment p_ptr to 3219801513
mkdirp:47: increment p_ptr to 3219801514
mkdirp:47: increment p_ptr to 3219801515
mkdirp:47: increment p_ptr to 3219801516
mkdirp:50: check for NULL path
mkdirp:57: check if path is absolute: '/        /'
mkdirp:65: p_ptr incremented to 3219801517, path ends at 3219801526
mkdirp:71: strchr(p_ptr, '/')
mkdirp:75: remove '/'
mkdirp:80: cur_path = '/        '
mkdirp:85: p_ptr = tmp_ptr+1 -> 3219801526, path ends at 3219801525

3219801526 > 3219801525

Another Example:
> ./mkdirp-test "        /        /     ////"
MAIN: path is '        /        /     ////'
mkdirp:17: strcpy('        /        /     ////')
mkdirp:20: set p_ptr to 3218805476 + 27 = 3218805503
mkdirp:22: removing trailing spaces in '        /        /     ////'
mkdirp:31: p_ptr = 3218805502, path = 3218805476
mkdirp:33: check for trailing '/' in '        /        /     ////'
mkdirp:40: p_ptr = path
mkdirp:43: removing leading spaces in '        /        /     ////'
mkdirp:47: increment p_ptr to 3218805477
mkdirp:47: increment p_ptr to 3218805478
mkdirp:47: increment p_ptr to 3218805479
mkdirp:47: increment p_ptr to 3218805480
mkdirp:47: increment p_ptr to 3218805481
mkdirp:47: increment p_ptr to 3218805482
mkdirp:47: increment p_ptr to 3218805483
mkdirp:47: increment p_ptr to 3218805484
mkdirp:50: check for NULL path
mkdirp:57: check if path is absolute: '/        /     ////'
mkdirp:65: p_ptr incremented to 3218805485, path ends at 3218805503
mkdirp:71: strchr(p_ptr, '/')
mkdirp:75: remove '/'
mkdirp:80: cur_path = '/        '
mkdirp:85: p_ptr = tmp_ptr+1 -> 3218805494, path ends at 3218805493
mkdirp:71: strchr(p_ptr, '/')
mkdirp:75: remove '/'
mkdirp:80: cur_path = '/        /     '
mkdirp:85: p_ptr = tmp_ptr+1 -> 3218805500, path ends at 3218805493
mkdirp:71: strchr(p_ptr, '/')
mkdirp:75: remove '/'
mkdirp:80: cur_path = '/        /     /'
mkdirp:85: p_ptr = tmp_ptr+1 -> 3218805501, path ends at 3218805493
mkdirp:71: strchr(p_ptr, '/')
mkdirp:75: remove '/'
mkdirp:80: cur_path = '/        /     //'
mkdirp:85: p_ptr = tmp_ptr+1 -> 3218805502, path ends at 3218805493
mkdirp:71: strchr(p_ptr, '/')
mkdirp:75: remove '/'
mkdirp:80: cur_path = '/        /     ///'
mkdirp:85: p_ptr = tmp_ptr+1 -> 3218805503, path ends at 3218805493


Generally spaces in pathnames are allowed and modifying/cleaning a pathname is not an easy task.


And the overflow:
>./mkdirp-test $(perl -e 'print "A" x 2000') 
[...]
mkdirp:57: check if path is absolute: 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADtà¿AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/'
mkdirp:60: no absolute path
Speicherzugriffsfehler

"Speicherzugriffsfehler" means SEGV :)

- before the main while-loop you do a p_ptr++, if path is "/" this will point
behind the buffer as far as I can see

This is not an issue. It just points to the end.

./pam done
next on the list: ./namcd

NSS_UNIXSTR_PATH should not point into /etc but into /var/run/namcd or similiar.

missed one comment for create_home.c:mkdirp():
- why is mkdir used with permission set 00777?

namcd.c:handler_worker():

	if (signum == SIGSEGV)
	{
//		/* This is a temporary fix for daemon getting killed because 
//			 of SIGSEGV in one of the worker threads. To solve this we 
//			 are catching this signal and killing that thread smoothly 
//			 and crating one more thread
//		 */
		pthread_t self = pthread_self();

		syslog(LOG_ERR, "SIGSEGV cought in this thread: [%d], restarting daemon \n", self);

		rerunDaemon();
	}


A segmentation fault is a very critical error. The program should not restart if it catches one. It even helps local attackers who can use this behaviour for brute-forcing the right offset needed to successfully exploit a possible buffer overflow.

The variable cur_signal is used in an asynchronous way, I think it is therefore better to declare it as
"volatile int cur_signal = 0;" instead of "int cur_signal = 0;"

namcd.c:init_threads():

Please replace

system("touch /var/lib/novell-lum/.namcdloaded > /dev/null 2>&1");

with

open(filename, O_CREAT)

regarding #c36 ... volatile is never the right solution except in some kernel drivers. you need to use the correct pthread locking primitives.

and #c37 ... open(filename, O_CREAT, 0600); (or whatever file permission) ;)

Marcus,
regarding the volatile I was thinking about the handling of signals which are async. but are queued one after another so no lock is needed for concurrent access like in threads. volatile just prevents the compiler from optimizing this variable which might have bad side-effects AFAIK.
But you are right, this variable is used within different threads and needs locking.

Nevertheless I missed one keyword to make operations atomic. "volatile sig_atomic_t ..."

JFYI. When I am done with namcd I will add a priority list of bugs.

packages approved. Thanks!

hrm, wrong bug#

One question. Is the user password handled by namcd stored as hash or as plaintext?

request.c:
- often values from the socket are accepted as trusted values and will be used as arguments for other functions without checking of type, range, ... For example:
int readFromSock(int sockfd, int flag, int *len, char **name ) 
{
...
	if (_nds_nss_readFromSock(sockfd,(char *)len, sizeof(int), NORESPONSE_TIMEOUT) != sizeof(int))
         ...
	if (flag==LENANDDATA)
	{
		int nb = *len,nbr;
		*name = (char*)malloc(nb);

- the above example is just a denial-of-service bug but some code line later it looks like it can be used to overflow buffer *name

     if ((nbr = _nds_nss_readFromSock(sockfd,*name, *len, NORESPONSE_TIMEOUT)) != nb)
     {
      ...


So, don't trust values read from the socket.

Hm, can everybody call the set-function setPwdForName() and polute the cache with it?

Created attachment 74859 [details]
0_SCAN.lint

Output of:

find novell-lum-2.2.0/ -name "*.c" -exec splint -warnposix +showcolumn +showsummary +stats +showfunc -namechecks +strict {} \; 2>&1 | tee 0_SCAN.lint

Just added this for the sake of completeness. Analysis of the output will be added later.

Created attachment 74920 [details]
0_SCAN_2.splint

for i in $(find novell-lum-2.2.0/ -name "*.c");do  echo "[ FILE: $i ]"; splint -warnposix -warnflags -preproc +forcehints +showsummary +stats -namechecks -exportconst  $i  2>/dev/null;echo "------------------------------------------------------------------------------------------";done > 0_SCAN_2.splint


Better to read and less noise.

Security-wise the clashes between the types are most interesting.

namcd/cache.c:
- the return values of the *alloc() functions should be checked everytime
- there are some sprintf(str,...) calls which can overflow even if you do malloc(len) before. len is a signed integer and can theoretically wrap around twice to be positive again by large passwd entries (like the gecos field which can be controlled by users sometimes)

will not review namcd/hash.c

namcd/nss_ldap.c
- some sprintf(filter, ...) calls use input from untrusted sources and may therefore overflow 'filter'
- note: the keyword 'access' which is reserved for access(2) is used as jump point
- the return values of the *alloc() functions should be checked everytime
- I am still very unsure about the usage of SLL in the code, can you please explain it to me?

summary of novell-lum (ordered by priority):
- check every sprintf(), strcpy, strcat() call for possible overflows
- no hardcoded passwords
- are SSL certificates verified? I need more info here.
- fix mkdirp()
- replace system() with C code
- do not trust vaues from sources you can't control (like socket input, certificates, user names, directories, hostnames, and so on) (like comment #45)
- check return values from important functions like *alloc()
- take care using /tmp
- use size_t for variables that hold sizes and length
- no constant salt values
- the split findings (will attach a truncated version in a few minutes)
- etc.

Created attachment 75134 [details]
0_SCAN_2_stripped.splint

Ok, novell-lum is done (just a quick check unfortunately).

TODO:
- more code checking
- runtime testing
- checking for design errors

after confering with Thomas and the rest of the team we come to the conclusion
that the points in #c52 need to be addressed before we can agree
to a Code10 inclusion of the code.

I addressed and checked in issues for following comments:

C#14, C#17, C#19, C#27 (added more explanation), C#29, C#33, C#34, C#37, C#39, 
C#49, C#51.

Answer for C#44: namcd stores password for users as MD5 hash.

Answer for C#27: get certificate from the server. More details below.

Answer for C#51 and C#52: CA is public certificate of the LDAP server.
This certificate (via APIs) does get extracted/requested by the namconfig utility during initial install/configuration of LUM and placed into /var/lib/novell-lum/ directory. When LDAP server and client are on the same side of the same firewall this is just extra convenience. If LDAP server and the client are not located inside of the same firewall this extracted certificate must be overwritten by certificate which was delivered via secure  trusted channel. 

Hm, where in the code is the signature chain of the cert verified, and where is the hostname (CN) in the cert compared with the remote peers address?

Created attachment 75596 [details]
0_SCAN.splint

novell-NLDAPsdk-8.8:

This splint output lists buffer overflows, format string bugs, memory leaks, assignment of different integer types, possible null reference, and so on.

./novell-NLDAPsdk-8.8/nlibraries/nldapx/ldapssl/src/ldapssl/debug.c:TraceStart()
- just a note:
336:   len = sprintf(temp, "%s tracing activated:\n"
337-                 " Start Time: %s\n"
338-                 " Version: %d.%d.%d %s\n"
--
349-                 __DATE__, __TIME__); 
350-
351:   assert(len < sizeof(temp)/sizeof(temp[0]));

detecting buffer overflows after they happened does not help.

fortunately the sprintf() parameters can't be influenced from the outside.

to answer my comment #57:

./novell-NLDAPsdk-8.8/nlibraries/nldapx/ldapssl/src/ldapssl/ssl_cert.c

verify_callback() does the job together with openssl.

SSL cert verification.

if openssl has a problem with verifying a cert it calls verify_callback().
verify_callback() calls cert_callback().
cert_callback() calls ldapssl_get_cert() to get a trusted root cert from eDirectory
and adds the cert using ldapssl_add_trusted_cert(). If adding the cert is successful
add_cert() propagates this success back to cert_callback() and further to 
verify_callback().

The cert, which was _invalid_, is accepted now _without_ any verifycation.

Result: SSL encryption is broken.

Is this correct?

Thomas, you may want create a separate entry for novell-NLDAPsdk and novell-NLDAPbase audit. The manager who owns the code is Rajkumar V from India Development Center. He need to be CCed or even be assigned this new entry.
Thanks. 

On comment #61. Please read carefully my response in c#56. Section "Answer for C#51 and C#52:"

I read your explanation about SSL handling but it didn't answer my question.

1st sentence makes no sense because CA means certification authority which is a organisational entity and not a certificate
2nd sentence describes how a certificate is received via another tool (this process might be vulnerable as well)
The rest includes no information at all.

Maybe I have a wrong picture of the process in my had.

The pam_lum module wants to talk to the LDAP server using SSL. pam_lum needs to verify the LDAP server cert received. If this server cert is signed with a special CA cert then this CA cert needs to be on the client side. I think this is the process you described in comment #56.

But verfiy_callback()->cert_callback() is called whenever the cert can't be verified to be trusted. Now you add a CA cert to your X509 storage which will be used *the next time* a server cert needs to be verified.

So, the next time you receive the server cert which is signed by the CA cert you added previously  the verification will be ok.

The bad thing is that you propagate the successful addition of the CA cert back to verify_callback() which just accepts this connection _without_ further verification. This happens for every untrusted server cert no matter how often you add the CA cert.

The code you are referring to (verify_callback() etc) will be invoked only if certificate is not found in /var/lib/novell-lum directory. The last one can occur for two reasons:

1. Administrator deleted the certificate by mistake (this is bad on his/her side).
or 
2. LDAP server and client are running in the controlled / trusted environment.
Then you may ask why even boder right? Well you still need to encrypt (the passwords on the wire for example) even though we trust the LDAP servers.

When you are referring to my 2nd sentence that the process may be vulnerable as well it is not true, unless admin is careless. The procedure to extract the certificate usually done using iManager or ConsoleOne utility from LDAP object in eDirectory. Then this certificate copied on the floppy and distributed to the LDAP clients or emailed via secure email or zipped with the password and emailed. In order to get to LDAP object in eDirectory admin has to login and also has to have enough rights to access the object. I hope this help.

Hm, we are talking about different things now. (BTW I would not assume any security policy of a network you can not control.)

The point is that cert_callback() is simply used wrong.

This callback function is used whenever a certificate can not be verified.
The *purpose* of this function is to show cert details to an user and let him/her interactively verfify the cert (for example by comparing the fingerprint).
You used it to add a CA cert which you *assume* is missing. This is not just a workaround but also breaks SSL security completely.

Please read "Viega, Messier, Chandra; Network Security with OpenSSL, O'Reilly", page 130 ff.

I think it is time for the call :-) Call me at 801-861-5839.

./novell-NLDAPsdk-8.8/nldapx/ldapgss/src/gss_bind.c:ldap_gssbind()
- possible buffer overflow: strcat(service_name, ldapHost);

The exploitablity depends on the code that uses this lib call.

ignore previous bug entry. it was the wrong bug.

Last entry in novell-lum.changes:
-------------------------------------------------------------------
Fri Mar 31 00:13:32 CEST 2006 - adanoyan@suse.de

- adding more entries per AJ request

- replaced number of strcpy, strcat, sprintf functions with strncpy, strncat, snprintf functions
(cache.c and nss_ldap.c)
- removed code (since it is not used anyway) with hardcoded proxy password for UWS
- fixed mkdirp()
- added code to check return code from *alloc calls where it was missing
- removed /tmp directory usage except the upgrade code
- moved UDS from /etc/ location to /var/lib/novell-lum location
- replaced system(touch... with open(...

-------------------------------------------------------------------

A lot of things got fixed.

I diff'ed the current package against the one I reviewed and will attach it in a few minutes.

One thing I missed above is _pam_log() in pam_ap.c can be made more bullet-proof.

static void _pam_log (int error, const char *format, ...)
{
	char buffer [1024];
	char identification [32];
	va_list args;

	sprintf (identification, "%s %s ", MODULE_NAME, VERSION);
	va_start (args, format);
	vsprintf (buffer, format, args); // XXX tom: use vsnprintf()
	openlog (identification, LOG_PID, LOG_AUTH);
	/*  openlog (identification, LOG_PID, LOG_LOCAL0); */
	setlogmask (LOG_UPTO (LOG_DEBUG));
	syslog (error, buffer); // XXX tom: format string bug: use syslog(error, "%s", buffer)
	closelog ();
}

Created attachment 76619 [details]
novell-lum.diff

This are issues which are left open.

summary:
- some sprintf and strcpy's that can be replaced
- constant salt values (does changing this mean a problem with the underlying structure of eDirectory or something a like?)
- directly using untrusted values
- the _pam_log() as mentioned above

Please specify sprintf and strcpy which need to be replaced.
Please specify places for untrusted values.

search for "XXX tom"

> Thu Apr  6 21:53:47 CEST 2006 - adanoyan@suse.de
>
> - replaced more strcpy, strcat, sprintf functions with strncpy, strncat, snprintf functions
> - removed number of system() calls
> - replaced system() call with C code
> - added full path to the parameter for the remaining system() calls
> - changed location for UDS from /var/lib/novell-lum to /var/run/novell-lum
> - added check for return code from calloc
> - added check on length of string
> - commented out call to ldapssl_set_verify_callback from pam and daemon code
> - fixed return error code in pam_sm_auth
> - commented out calls to set LM and NT passwords, note used for now
> - added checks for the range of values comming via UDS, because they are untrusted
> - fixed bug in _pam_log()
> - modified init script for daemon to use startproc and killproc utilities

Submitted into Beta10 STABLE

TODO:

- look at copy_tree() 
- check itoa()
- look into random salt
- on the et commands over UDS pass and check UID for user to prevent hash password pollution.

Thanks a lot for fixing this so fast!

I forgot about Comment #45 and setPwdForName().

Can everybody on the system use it?

If so what happens when I set the password for user 'joe' and execute "su - joe"?

Yes, this is what I am going to do next. See my TODO list Comment #76 !

I saw your todo list. :)

But if this bug is serious like this it can't wait for SP1.

Hey Thomas, the TODO list is NOT SP1 list. :-) 
Otherwise I would call it SP1 list.
This is just reminder to myself what still needs to be done.
Do not worry !

Ok, new code drop looks good.

But cert_callback() is still used:

thomas> find novell-lum-2.2.0/ -name "*.c" -exec grep --with-filename set_verify_callback {} \;

novell-lum-2.2.0/config/confuncs.c:          ret_val = ldapssl_set_verify_callback(cert_callback);
novell-lum-2.2.0/unix2edir/unix2edir.c:                      err=ldapssl_set_verify_callback(cert_callback);

The first case is used by namconfig; we made an exception here as long as we document it.
The current namconfig.1 doesn't explain this behavior (which is still an insecure workaround) to the admin.

The code in unix2edit should be removed.

Wed Apr 12 04:40:58 CEST 2006 - adanoyan@suse.de

- added code to check on identity of the person who is trying to set hash password in the LUM cache
- commented out certcallback in unix2edir utility
- added more information in man pages for namconfig about the certificate file

Code looks good now.

the following issues are left:
- don't restart on SEGV, better fix the bug ;)
- look at copy_tree() 
- check itoa()
- look into random salt
- replace system() with C-code

Alex,
do you have something else to add to the list?

This list looks good. 
Are we OK to move this list to SP1 ?

Yes, it's ok.

What is the status of this bug? ready to be closed?

Maybe Alex needs the SP1 list... Alex?

From my side all the important bugs are fixed.

I created a new bug #178647 to capture the work which need to be done for SP1.
I am OK to close this one.

Ah, I was too quick.

The code was already released for the following distributions:
thomas@bragg:~> is_maintained novell-lum
Package is on CD sled10.i386
        Distribution: sles10-i386
        Distributionstring: SUSE-Linux-SLES-i386
        Marketing-Name: SUSE SLED 10 for x86
Package is on CD sled10.x86_64
        Distribution: sles10-x86_64
        Distributionstring: SUSE-Linux-SLES-x86-64
        Marketing-Name: SUSE SLED 10 for AMD64 and Intel EM64T
Package is on CD sles9-oes.i386
        Distribution: sles9-i386
        Distributionstring: Novell-Open-Enterprise-Server-i386
        Marketing-Name: Open Enterprise Server


Correct me if I am wrong.

At least it is true for OES which needs an update then.

MaintenanceTracker-4434

Thomas, I do not understand your question. This code already checked into OES (sles 9 based) version conrol reposotory in addition to autobuild for SLED10 and will be released as part of OES SP3.  

Can you tell me when OES SP3 will be released?

BTW, I requested some CVE numbers from Mitre (cve.mitre.org). Try to use them in changelogs and update texts as a common cross-reference please.


>       + fixed several possible buffer overflows

CVE-2006-2622

>       + fixed SSL certificate handling to avoid man-in-the-middle attack

CVE-2006-2623

>       + fixed possible format string bug in PAM function

CVE-2006-2624

>       + poisoning credential cache can lead to local privilege escalation

CVE-2006-2625

Thomas, I do not know when OES SP3 will be released. I asked a few people
who I thought could know, but they did not know either.

Hm, this leaves customers open for attacks for an unknown period of time.

Are you familiar with online updates? Have a look at 
http://w3d.suse.de./Dev//Components/Packages/PackMan

Regarding the severity of the bugs and the unavailability of a SP3 release in near future an online update for all affected version should be supplied.

Let's forget the online update. Changes in the code are too complex a the customers system may not work anymore.

So, let's do it for SP3. Is there something we need to take care of to submit this code to SP3? I saw novell-lum submits to 10.1 and STABLE (do we see OES9 submits here in Nbg.? *shrug*).

reassigned to Alex to take care of SP3 submit..

Alex,
My apologies, I thought I had already sent you Email stating the patch was released.  For customers, the patch is number:
11207   novell-lum      A       Security patch
This was released       9/29/2006.

It is available on the oes public channel.
Let me know if you have any other questions. 

>>> Alexander Danoyan 10/9/2006 7:32 PM >>>
Hi All,

How can I find out if this patch was placed to RC server to the world.
I checked the SWAMP server, but it does not tell me much.
It just says the status is inactive.
https://swamp.suse.de/webswamp/swamp/template/DisplayWorkflow.vm/workflowid/6032/history/1/historyType/Tasks
Thanks.

Alex.

asked Steven from Mitre to make CVE-IDs public.