Skip to content

pppd: avoid TOCTOU in protected file access #592

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
LGA1150 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

pppd: avoid TOCTOU in protected file access #592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ AC_CHECK_SIZEOF(unsigned short)

# Checks for library functions.
AC_CHECK_FUNCS([ \
fexecve \
mmap \
logwtmp \
strerror])
Expand Down
146 changes: 83 additions & 63 deletions pppd/auth.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ static int setupapfile (char **);
static int privgroup (char **);
static int set_noauth_addr (char **);
static int set_permitted_number (char **);
static void check_access (FILE *, char *);
static void check_access (int, const char *);
Copy link
Copy Markdown
Contributor

@jkroonza jkroonza May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this was still in place after previous patch set, but since this is now already checked by ppp_check_access I think this function can go away entirely. I think you modified this now based on my previous review noting that ppp_check_access was then taking a FILE*.

I think in order to not just fix the patch but actually make progress too regarding simplification, kill this function entirely too please unless I'm being stupid and it does actually check something which I'm missing, but as far as I can tell it doesn't actually enforce anything other than warning about the file being group/other writeable - something which is now enforced already by ppp_check_access.

Copy link
Copy Markdown
Collaborator

@paulusmack paulusmack Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it warns about the file being group/other readable so that is different to ppp_check_access. But this function could easily go away, making a nice simplification, with an extra parameter to ppp_check_access telling it to check group/other read permissions.

static int wordlist_count (struct wordlist *);
static void check_maxoctets (void *);

Expand Down Expand Up @@ -532,7 +532,7 @@ setupapfile(char **argv)
free(fname);
return 0;
}
check_access(ufile, fname);
check_access(fileno(ufile), fname);
uafname = fname;

/* get username */
Expand Down Expand Up @@ -1543,16 +1543,21 @@ check_passwd(int unit,
* Open the file of pap secrets and scan for a suitable secret
* for authenticating this user.
*/
if (!ppp_check_access(path_upapfile, &filename, 0, 0))
return UPAP_AUTHNAK;
filename = path_upapfile;
Copy link
Copy Markdown
Contributor

@jkroonza jkroonza May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that this is a straight up revert, so if you don't mind, please fix in this commit - so either you can clean up now or I'll post a follow-up clean up patch where I can find this idiom.

Cases like this is not ideal either so a follow up commit trying to trace and fix all of them is possibly better:

2546 have_eaptls_secret_client()

2550 char *filename;

2567 filename = PPP_PATH_EAPTLSCLIFILE;

Plainly we're assigning const char* there to char* which can lead to hard to troubleshoot issues.

Copy link
Copy Markdown
Contributor Author

@LGA1150 LGA1150 May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A subsequent clean-up patch could constify all path variables as long as parameters.

addrs = opts = NULL;
ret = UPAP_AUTHNAK;
f = fopen(filename, "r");
if (f == NULL) {
error("Can't open PAP password file %s: %m", filename);

} else {
check_access(f, filename);
int fd = fileno(f);

if (!ppp_check_access(fd, filename, 0)) {
Copy link
Copy Markdown
Contributor

@jkroonza jkroonza May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once check_access() goes away I think fileno() inline here is fine avoiding the extra variable.

fclose(f);
return UPAP_AUTHNAK;
}
check_access(fd, filename);
if (scan_authfile(f, user, our_name, secret, &addrs, &opts, filename, 0) < 0) {
warn("no PAP secret found for %s", user);
} else {
Expand Down Expand Up @@ -1586,7 +1591,6 @@ check_passwd(int unit,
}
fclose(f);
}
free(filename);

if (ret == UPAP_AUTHNAK) {
if (**msg == 0)
Expand Down Expand Up @@ -1646,21 +1650,24 @@ null_login(int unit)
* Open the file of pap secrets and scan for a suitable secret.
*/
if (ret <= 0) {
if (!ppp_check_access(path_upapfile, &filename, 0, 0))
return 0;
int fd;

filename = path_upapfile;
addrs = NULL;
f = fopen(filename, "r");
if (f == NULL) {
free(filename);
if (f == NULL)
return 0;
fd = fileno(f);
if (!ppp_check_access(fd, filename, 0)) {
fclose(f);
return 0;
}
check_access(f, filename);
check_access(fd, filename);

i = scan_authfile(f, "", our_name, secret, &addrs, &opts, filename, 0);
ret = i >= 0 && secret[0] == 0;
BZERO(secret, sizeof(secret));
fclose(f);
free(filename);
}

if (ret)
Expand Down Expand Up @@ -1701,7 +1708,7 @@ get_pap_passwd(char *passwd)
f = fopen(filename, "r");
if (f == NULL)
return 0;
check_access(f, filename);
check_access(fileno(f), filename);
ret = scan_authfile(f, user,
(remote_name[0]? remote_name: NULL),
secret, NULL, NULL, filename, 0);
Expand Down Expand Up @@ -1734,18 +1741,18 @@ have_pap_secret(int *lacks_ipp)
return ret;
}

if (!ppp_check_access(path_upapfile, &filename, 0, 0))
return 0;
filename = path_upapfile;
f = fopen(filename, "r");
if (f == NULL) {
free(filename);
if (f == NULL)
return 0;
}

if (!ppp_check_access(fileno(f), filename, 0)) {
fclose(f);
return 0;
}
ret = scan_authfile(f, (explicit_remote? remote_name: NULL), our_name,
NULL, &addrs, NULL, filename, 0);
fclose(f);
free(filename);
if (ret >= 0 && !some_ip_ok(addrs)) {
if (lacks_ipp != 0)
*lacks_ipp = 1;
Expand Down Expand Up @@ -1780,11 +1787,13 @@ have_chap_secret(char *client, char *server,
}
}

if (!ppp_check_access(path_chapfile, &filename, 0, 0))
return 0;
filename = path_chapfile;
f = fopen(filename, "r");
if (f == NULL) {
free(filename);
if (f == NULL)
return 0;

if (!ppp_check_access(fileno(f), filename, 0)) {
fclose(f);
return 0;
}

Expand All @@ -1795,7 +1804,6 @@ have_chap_secret(char *client, char *server,

ret = scan_authfile(f, client, server, NULL, &addrs, NULL, filename, 0);
fclose(f);
free(filename);
if (ret >= 0 && need_ip && !some_ip_ok(addrs)) {
if (lacks_ipp != 0)
*lacks_ipp = 1;
Expand All @@ -1822,11 +1830,13 @@ have_srp_secret(char *client, char *server, int need_ip, int *lacks_ipp)
char *filename;
struct wordlist *addrs;

if (!ppp_check_access(PPP_PATH_SRPFILE, &filename, 0, 0))
return 0;
filename = PPP_PATH_SRPFILE;
f = fopen(filename, "r");
if (f == NULL) {
free(filename);
if (f == NULL)
return 0;

if (!ppp_check_access(fileno(f), filename, 0)) {
fclose(f);
return 0;
}

Expand All @@ -1837,7 +1847,6 @@ have_srp_secret(char *client, char *server, int need_ip, int *lacks_ipp)

ret = scan_authfile(f, client, server, NULL, &addrs, NULL, filename, 0);
fclose(f);
free(filename);
if (ret >= 0 && need_ip && !some_ip_ok(addrs)) {
if (lacks_ipp != 0)
*lacks_ipp = 1;
Expand Down Expand Up @@ -1874,22 +1883,27 @@ get_secret(int unit, char *client, char *server,
return 0;
}
} else {
if (!ppp_check_access(path_chapfile, &filename, 0, 0))
return 0;
int fd;

filename = path_chapfile;
addrs = NULL;
secbuf[0] = 0;

f = fopen(filename, "r");
if (f == NULL) {
error("Can't open chap secret file %s: %m", filename);
free(filename);
return 0;
}
check_access(f, filename);

fd = fileno(f);
if (!ppp_check_access(fd, filename, 0)) {
fclose(f);
return 0;
}
check_access(fd, filename);

ret = scan_authfile(f, client, server, secbuf, &addrs, &opts, filename, 0);
fclose(f);
free(filename);
if (ret < 0)
return 0;

Expand Down Expand Up @@ -1931,23 +1945,27 @@ get_srp_secret(int unit, char *client, char *server,
if (!am_server && passwd[0] != '\0') {
strlcpy(secret, passwd, MAXWORDLEN);
} else {
if (!ppp_check_access(PPP_PATH_SRPFILE, &filename, 0, 0))
return 0;
addrs = NULL;
int fd;

filename = PPP_PATH_SRPFILE;
addrs = NULL;
fp = fopen(filename, "r");
if (fp == NULL) {
error("Can't open srp secret file %s: %m", filename);
free(filename);
return 0;
}
check_access(fp, filename);

fd = fileno(fp);
if (!ppp_check_access(fd, filename, 0)) {
fclose(fp);
return 0;
}
check_access(fd, filename);

secret[0] = '\0';
ret = scan_authfile(fp, client, server, secret, &addrs, &opts,
filename, am_server);
fclose(fp);
free(filename);
if (ret < 0)
return 0;

Expand Down Expand Up @@ -2214,11 +2232,11 @@ auth_number(void)
* check_access - complain if a secret file has too-liberal permissions.
*/
static void
check_access(FILE *f, char *filename)
check_access(int fd, const char *filename)
{
struct stat sbuf;

if (fstat(fileno(f), &sbuf) < 0) {
if (fstat(fd, &sbuf) < 0) {
warn("cannot stat secret file %s: %m", filename);
} else if ((sbuf.st_mode & (S_IRWXG | S_IRWXO)) != 0) {
warn("Warning - secret file %s has world and/or group access",
Expand Down Expand Up @@ -2326,25 +2344,25 @@ scan_authfile(FILE *f, char *client, char *server,
* Special syntax: @/pathname means read secret from file.
*/
if (word[0] == '@' && word[1] == '/') {
char *realname;
int fd;

strlcpy(atfile, word+1, sizeof(atfile));
if (!ppp_check_access(atfile, &realname, 0, 0))
continue;
if ((sf = fopen(realname, "r")) == NULL) {
if ((sf = fopen(atfile, "r")) == NULL) {
warn("can't open indirect secret file %s", atfile);
free(realname);
continue;
}
check_access(sf, atfile);
fd = fileno(sf);
if (!ppp_check_access(fd, atfile, 0)) {
fclose(sf);
continue;
}
check_access(fd, atfile);
if (!getword(sf, word, &xxx, atfile)) {
warn("no secret in indirect secret file %s", atfile);
fclose(sf);
free(realname);
continue;
}
fclose(sf);
free(realname);
}
strlcpy(lsecret, word, sizeof(lsecret));
}
Expand Down Expand Up @@ -2503,14 +2521,15 @@ have_eaptls_secret_server(char *client, char *server,
char cacertfile[MAXWORDLEN];
char pkfile[MAXWORDLEN];

if (!ppp_check_access(PPP_PATH_EAPTLSSERVFILE, &filename, 0, 0))
return 0;
filename = PPP_PATH_EAPTLSSERVFILE;
f = fopen(filename, "r");
if (f == NULL) {
free(filename);
if (f == NULL)
return 0;
}

if (!ppp_check_access(fileno(f), filename, 0)) {
fclose(f);
return 0;
}
if (client != NULL && client[0] == 0)
client = NULL;
else if (server != NULL && server[0] == 0)
Expand All @@ -2522,7 +2541,6 @@ have_eaptls_secret_server(char *client, char *server,
0);

fclose(f);
free(filename);

/*
if (ret >= 0 && !eaptls_init_ssl(1, cacertfile, servcertfile,
Expand Down Expand Up @@ -2782,26 +2800,28 @@ get_eaptls_secret(int unit, char *client, char *server,
}
else
{
int fd;

filename = (am_server ? PPP_PATH_EAPTLSSERVFILE : PPP_PATH_EAPTLSCLIFILE);
addrs = NULL;

if (!ppp_check_access(filename, &filename, 0, 0))
return 0;
fp = fopen(filename, "r");
if (fp == NULL)
{
if (fp == NULL) {
error("Can't open eap-tls secret file %s: %m", filename);
free(filename);
return 0;
}

check_access(fp, filename);
fd = fileno(fp);
if (!ppp_check_access(fd, filename, 0)) {
fclose(fp);
return 0;
}
check_access(fd, filename);

ret = scan_authfile_eaptls(fp, client, server, clicertfile, servcertfile,
cacertfile, pkfile, &addrs, &opts, filename, 0);

fclose(fp);
free(filename);

if (ret < 0) return 0;
}
Expand Down
Loading
Loading