Skip to content

Commit d0c5309

Browse files
committed
demux_mf: improve format string processing
Before this commit, the user could specify a printf format string which wasn't verified, and could result in: - Undefined behavior due to missing or non-matching arguments. - Buffer overflow due to untested result length. The offending code was added at commit 103a960 (2002, mplayer svn): git-svn-id: svn://svn.mplayerhq.hu/mplayer/trunk@4566 b3059339-0415-0410-9bf9-f77b7e298cf2 It moved around but was not modified meaningfully until now. Now we reject all conversion specifiers at the format except %% and a simple subset of the valid specifiers. Also, we now use snprintf to avoid buffer overflow. The format string is provided by the user as part of mf:// URI. Report and initial patch by Stefan Schiller. Patch reviewed by @jeeb, @sfan5, Stefan Schiller.
1 parent ef9596f commit d0c5309

File tree

1 file changed

+37
-2
lines changed

1 file changed

+37
-2
lines changed

demux/demux_mf.c

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,8 @@ static mf_t *open_mf_pattern(void *talloc_ctx, struct demuxer *d, char *filename
121121
goto exit_mf;
122122
}
123123

124-
char *fname = talloc_size(mf, strlen(filename) + 32);
124+
size_t fname_avail = strlen(filename) + 32;
125+
char *fname = talloc_size(mf, fname_avail);
125126

126127
#if HAVE_GLOB
127128
if (!strchr(filename, '%')) {
@@ -148,10 +149,44 @@ static mf_t *open_mf_pattern(void *talloc_ctx, struct demuxer *d, char *filename
148149
}
149150
#endif
150151

152+
// We're using arbitrary user input as printf format with 1 int argument.
153+
// Any format which uses exactly 1 int argument would be valid, but for
154+
// simplicity we reject all conversion specifiers except %% and simple
155+
// integer specifier: %[.][NUM]d where NUM is 1-3 digits (%.d is valid)
156+
const char *f = filename;
157+
int MAXDIGS = 3, nspec = 0, bad_spec = 0, c;
158+
159+
while (nspec < 2 && (c = *f++)) {
160+
if (c != '%')
161+
continue;
162+
if (*f != '%') {
163+
nspec++; // conversion specifier which isn't %%
164+
if (*f == '.')
165+
f++;
166+
for (int ndig = 0; mp_isdigit(*f) && ndig < MAXDIGS; ndig++, f++)
167+
/* no-op */;
168+
if (*f != 'd') {
169+
bad_spec++; // not int, or beyond our validation capacity
170+
break;
171+
}
172+
}
173+
// *f is '%' or 'd'
174+
f++;
175+
}
176+
177+
// nspec==0 (zero specifiers) is rejected because fname wouldn't advance.
178+
if (bad_spec || nspec != 1) {
179+
mp_err(log, "unsupported expr format: '%s'\n", filename);
180+
goto exit_mf;
181+
}
182+
151183
mp_info(log, "search expr: %s\n", filename);
152184

153185
while (error_count < 5) {
154-
sprintf(fname, filename, count++);
186+
if (snprintf(fname, fname_avail, filename, count++) >= fname_avail) {
187+
mp_err(log, "format result too long: '%s'\n", filename);
188+
goto exit_mf;
189+
}
155190
if (!mp_path_exists(fname)) {
156191
error_count++;
157192
mp_verbose(log, "file not found: '%s'\n", fname);

0 commit comments

Comments
 (0)