Skip to content

Commit bd1c86a

Browse files
committed
passwd-file parsing rewrite
as i suspected, the pointer math when stepping through the passwd file was not quite up to snuff.
1 parent 2199fdc commit bd1c86a

File tree

3 files changed

+61
-57
lines changed

3 files changed

+61
-57
lines changed

bugs.txt

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,13 @@ rfc 2617
2424
though it only exists as a convenience to stash state in, but i could believe some software
2525
out there depends upon it...
2626

27+
- apparently (older?) versions of internet explorer don't obey the spec when dealing with
28+
paths that contain a query string. the client should include the query in the url used
29+
to compute the digest, but IE truncates it at the question mark. see ‘current browser
30+
issues’ comment here for details: http://www.xiven.com/sourcecode/digestauthentication.php
31+
2732

2833
general (in)security
29-
- i followed the model of the auth_basic module which pages through the password file's
30-
contents on every request. i know from experience that it's impossible for me to write that
31-
sliding-window-through-a-buffer routine without a creating at least a couple off-by-one
32-
errors and non-terminated strings. it would be nice to find them.
33-
3434
- OOM conditions in the shm segment are not handled at all well at the moment leading to an
3535
easy DOS attack (presuming the shm size is set low enough to be exhaustible within the timeout
3636
+ expire interval). Valid nonces are added to the shm and expired seconds or minutes later.

ngx_http_auth_digest_module.c

Lines changed: 55 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -140,18 +140,25 @@ ngx_http_auth_digest_handler(ngx_http_request_t *r)
140140
ngx_int_t rc;
141141
ngx_err_t err;
142142
ngx_str_t user_file, passwd_line;
143-
ngx_uint_t i, level, left;
144143
ngx_file_t file;
144+
ngx_uint_t i, begin, tail, idle;
145145
ngx_http_auth_digest_loc_conf_t *alcf;
146146
ngx_http_auth_digest_cred_t *auth_fields;
147147
u_char buf[NGX_HTTP_AUTH_DIGEST_BUF_SIZE];
148-
148+
u_char line[NGX_HTTP_AUTH_DIGEST_BUF_SIZE];
149+
u_char *p;
149150

150151

151152
// if digest auth is disabled for this location, bail out immediately
152153
alcf = ngx_http_get_module_loc_conf(r, ngx_http_auth_digest_module);
153154
if (alcf->realm.len == 0 || alcf->user_file.value.len == 0) {
154155
return NGX_DECLINED;
156+
157+
//
158+
// BUG? wait wait wait. shouldn't this be ngx_ok by default in the case
159+
// of the former and ngx_declined in the latter?
160+
//
161+
155162
}
156163

157164
// unpack the Authorization header (if any) and verify that it contains all
@@ -164,12 +171,13 @@ ngx_http_auth_digest_handler(ngx_http_request_t *r)
164171
return NGX_HTTP_INTERNAL_SERVER_ERROR;
165172
}
166173

167-
// read in the passwd file from disk
174+
// check for the existence of a passwd file and attempt to open it
168175
if (ngx_http_complex_value(r, &alcf->user_file, &user_file) != NGX_OK) {
169176
return NGX_ERROR;
170177
}
171178
fd = ngx_open_file(user_file.data, NGX_FILE_RDONLY, NGX_FILE_OPEN, 0);
172179
if (fd == NGX_INVALID_FILE) {
180+
ngx_uint_t level;
173181
err = ngx_errno;
174182

175183
if (err == NGX_ENOENT) {
@@ -185,76 +193,72 @@ ngx_http_auth_digest_handler(ngx_http_request_t *r)
185193
ngx_open_file_n " \"%s\" failed", user_file.data);
186194
return rc;
187195
}
188-
189196
ngx_memzero(&file, sizeof(ngx_file_t));
190-
191197
file.fd = fd;
192198
file.name = user_file;
193199
file.log = r->connection->log;
194200

195-
// parse through the passwd file and find the individual lines, then pass them off
201+
// step through the passwd file and find the individual lines, then pass them off
196202
// to be compared against the values in the authorization header
197-
left = offset = 0;
203+
passwd_line.data = line;
204+
offset = begin = tail = 0;
205+
idle = 1;
206+
ngx_memzero(buf, NGX_HTTP_AUTH_DIGEST_BUF_SIZE);
207+
ngx_memzero(passwd_line.data, NGX_HTTP_AUTH_DIGEST_BUF_SIZE);
198208
while (1){
199-
n = ngx_read_file(&file, buf+left, NGX_HTTP_AUTH_DIGEST_BUF_SIZE-left, offset);
200-
201-
if (n==0){
202-
ngx_str_t remain;
203-
remain.len = left;
204-
remain.data = buf;
205-
remain.data[left] = '\0';
206-
207-
rc = ngx_http_auth_digest_verify_user(r, auth_fields, &remain);
208-
if (rc!=NGX_DECLINED){
209-
ngx_http_auth_digest_close(&file);
210-
return rc;
211-
}
212-
213-
break;
214-
}
215-
209+
n = ngx_read_file(&file, buf+tail, NGX_HTTP_AUTH_DIGEST_BUF_SIZE-tail, offset);
216210
if (n == NGX_ERROR) {
217211
ngx_http_auth_digest_close(&file);
218212
return NGX_HTTP_INTERNAL_SERVER_ERROR;
219213
}
220-
221-
left = 0;
222-
for (i=left; i<(ngx_uint_t)n; i++){
223-
if (i==left && (buf[i]==CR||buf[i]==LF||buf[i]=='\0')){
224-
left++;
225-
continue;
226-
}
227-
228-
if (buf[i] == CR || buf[i] == LF){
229-
u_char *p;
230-
passwd_line.len = i - left + 1;
231-
passwd_line.data = ngx_pcalloc(r->pool, passwd_line.len);
232-
if (passwd_line.data==NULL){
233-
ngx_http_auth_digest_close(&file);
234-
return NGX_HTTP_INTERNAL_SERVER_ERROR;
214+
215+
begin = 0;
216+
for (i=0; i<n+tail; i++){
217+
if (buf[i] == '\n' || buf[i] == '\r'){
218+
if (!idle && i-begin>36){ // 36 is the min length with a single-char name and realm
219+
p = ngx_cpymem(passwd_line.data, &buf[begin], i-begin);
220+
p[0] = '\0';
221+
passwd_line.len = i-begin;
222+
rc = ngx_http_auth_digest_verify_user(r, auth_fields, &passwd_line);
223+
if (rc != NGX_DECLINED){
224+
ngx_http_auth_digest_close(&file);
225+
return rc;
226+
}
235227
}
236-
p = ngx_cpymem(passwd_line.data, &buf[left], i-left);
237-
228+
idle = 1;
229+
begin = i;
230+
}else if(idle){
231+
idle = 0;
232+
begin = i;
233+
}
234+
}
235+
236+
if (!idle){
237+
tail = n + tail - begin;
238+
if (n==0 && tail>36){
239+
p = ngx_cpymem(passwd_line.data, &buf[begin], tail);
240+
p[0] = '\0';
241+
passwd_line.len = i-begin;
238242
rc = ngx_http_auth_digest_verify_user(r, auth_fields, &passwd_line);
239243
if (rc != NGX_DECLINED){
240244
ngx_http_auth_digest_close(&file);
241245
return rc;
242-
}
243-
left = i+1;
246+
}
247+
}else{
248+
ngx_memmove(buf, &buf[begin], tail);
244249
}
245250
}
251+
252+
if (n==0){
253+
break;
254+
}
246255

247-
if (left<=(ngx_uint_t)n){
248-
ngx_memmove(buf, &buf[left], n-left);
249-
left = n-left;
250-
}else{
251-
left = 0;
252-
}
253-
offset+=n;
256+
offset += n;
254257
}
258+
255259
ngx_http_auth_digest_close(&file);
256260

257-
// no match was found based on the fields in the authorization header.
261+
// since no match was found based on the fields in the authorization header,
258262
// send a new challenge and let the client retry
259263
return ngx_http_auth_digest_send_challenge(r, &alcf->realm, auth_fields->stale);
260264
}

ngx_http_auth_digest_module.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ static ngx_int_t ngx_http_auth_digest_handler(ngx_http_request_t *r);
4444
// passwd file handling
4545
static void ngx_http_auth_digest_close(ngx_file_t *file);
4646
static char *ngx_http_auth_digest_user_file(ngx_conf_t *cf, ngx_command_t *cmd, void *conf);
47-
#define NGX_HTTP_AUTH_DIGEST_BUF_SIZE 2048
47+
#define NGX_HTTP_AUTH_DIGEST_BUF_SIZE 4096
4848

4949
// digest challenge generation
5050
static ngx_int_t ngx_http_auth_digest_send_challenge(ngx_http_request_t *r,

0 commit comments

Comments
 (0)