htsserver: an unauthenticated GET of a directory spins the server forever#724
Merged
Conversation
fopen() succeeds on a directory on POSIX and every read from it fails with EISDIR without ever raising EOF, so smallserver()'s "while (!feof(fp))" serving loop never terminates. GET /server/ needs no session id and no project to reach it, and the accept loop is single-threaded, so one unauthenticated local request wedges WebHTTrack for good. Refuse a directory before fopen() so the 404 branch answers, rather than only ending the loop: a loop-only fix would serve every directory as an empty 200. The two loops fed a client-influenced path also stop on ferror(), since a read that fails for any other reason spins the same way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
fexist() (htsserver.h) is the stat + S_ISREG predicate this file already uses for the "file-exists:" template op, so gate the serving fopen() on it rather than on a fresh negated is_directory(). Refusing only directories still handed FIFOs to the same code path, where fopen() blocks with no writer and kills the single-threaded accept loop for good. Test 91 gains the FIFO case and a POST that loads a project whose hts-cache/winprofile.ini is a directory: that fopen() sits ahead of every guard, so it covers the ferror() check on the project-load read loop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
…ry-loop # Conflicts: # tests/Makefile.am
| && (fp = fopen(fsfile, "rb"))) { | ||
| /* Regular files only: serving a directory or a FIFO never ends. */ | ||
| if (fsfile[0] && strstr(file, "..") == NULL && fexist(fsfile) && | ||
| (fp = fopen(fsfile, "rb"))) { |
…ry-loop Signed-off-by: Xavier Roche <roche@httrack.com> # Conflicts: # tests/Makefile.am
…ry-loop # Conflicts: # tests/Makefile.am
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GET /server/wedges a freshly started htsserver: no session id, no project, nothing set up.fopen()on a directory succeeds on POSIX and every read from it then fails with EISDIR without ever setting the EOF flag, so thewhile (!feof(fp))loop that serves a static file insmallserver()never terminates. The session id gates the request body, not GET, and connections come off a single-threaded accept loop, so one request from localhost pins the process at 100% CPU and it never answers anything again./website/does the same once a project is registered, and that is where the GUI's "browse mirrored site" link goes.The serving
fopen()now runs only for a regular file, gated onfexist(), the stat +S_ISREGpredicate this file already applies to thefile-exists:template op. Refusing directories alone would leave FIFOs on the same path, wherefopen()blocks for want of a writer and kills the accept loop just as dead, so a whitelist closes the class instead of one instance. Ending the read loop rather than refusing the open is not an alternative: every directory would then come back as a well-formed200 OKwith an empty body, which is worse than a 404 for a mirror browser. One behavior change falls out of it./dev/nullunder a mirror root now 404s instead of serving zero bytes, and that seems right: a mirror holds what HTTrack wrote there, and that is never a device node.The settings loader
fopen()s the postedpath+projnameahead of every guard, so its read loop stops onferror()too. The GUI template loop and thefreadserving loop got the same treatment, but both sit behind the whitelisted open and I have no probe that reaches them: they are defense in depth with no coverage of their own.tests/91_webhttrack-directory.testasks for/server/,/website/, a subdirectory and a FIFO, then POSTs a project load whosehts-cache/winprofile.iniis a directory, with a positive control that a real mirror file still serves and an aliveness check after each. Restoring the directory-only refusal fails it on the FIFO alone; restoringwhile (!feof(fp))on the settings loader fails it on the project load. The red CodeQL check is pre-existing alert #195,cpp/path-injectionon this samefopen, open on master since 2026-07-07 and renumbered by the added lines.