[Swan-dev] Fix bug in do_file_authentication

[D. Hugh Redelmeier]

| From: David McCullough <ucdevel at gmail.com>
| Paul Wouters wrote the following:
| > On Fri, 4 Apr 2014, D. Hugh Redelmeier wrote:
| > 
| > >   Fix bug in do_file_authentication.
| > >   Introduced by DHR on 2014 Jan 12 via 69caecc522448a4c36d679d0f3ca48c0864b2182.
| > 
| > I'm still confused here, the old/new code is:
| > 
| >                 /* get userid */
| >                 userid = line;
| >                 p = strchr(p, ':');     /* find end */
| 
| This should be
| 
| 		p = strchr(line, ':');

Right.  Thanks!

When that line starts, p points at a NUL at the end of line
(unless the line didn't end with a newline).  It's never going to find
a colon without David's fix.

At least it would no longer crash (unless the line didn't end with a newline).

| >                 if (p == NULL) {
| >                         /* no end: skip line */
| >                         libreswan_log("XAUTH: %s:%d missing password hash field", pwdfile, lineno);
| >                         continue;
| >                 }
| > 
| >                 *p++ ='\0';     /* terminate string by overwriting : */
| > 
| > At this last line, p was pointing to 0x01. How can strchr ever return
| > such an invalid memory pointer? So I also don't trust the test for p !=
| > NULL and running *p++ ='\0';
| > 
| > What did I get wrong?

In the crash case, p was NULL coming out of the strchr (meaning "not found").

"*p++ = '0';" crashed, referencing *NULL, but before the crash
actually happened, the ++ happened, so NULL turned into
0x0000000000000001

The test that I (partly) fixed prevents control reaching this
statement if p == NULL.  It logs the error message instead.