Read every address parameter through one parser (#76) - #77
Merged
Conversation
SRVIP, PASVADR and PASVBIND were parsed three different ways, with three
different ideas of what an unreadable value means. SRVIP was the worst of
them: the value was copied verbatim, so the comma form the other two accept
made sscanf("%u.%u.%u.%u") report one conversion instead of four, the else
branch did nothing, and s_addr kept the 0 from the memset. SRVIP=127,0,0,1
bound every address of the host without a word -- the exact opposite of what
the operator asked for, and the setup the sample recommends for running
behind a TLS proxy (#59). A typo did the same: SRVIP=127.0.0 or
SRVIP=localhost were just as silent.
ftpd_adr_parse() now reads all three: ANY, dotted quad or comma form, four
octets, 0-255, nothing trailing. It also closes holes the sscanf version
left open for PASVBIND -- 999.1.2.3 passed as four conversions, 1.2.3.4junk
too, and a value longer than the buffer was truncated into a valid but
different address (192.168.100.2000 -> 192.168.100.200). Rejected values
are refused with a message naming the default that takes their place, and
the listener refuses to start rather than fall through to INADDR_ANY.
SRVBIND is the new name; SRVIP is still read and the CONFIG dump says when
it was the spelling that took effect. PASVADR keeps its name -- it is the
advertised address, not a bind address, and that difference is worth
keeping visible.
Its silent getsockname() fallback becomes the named default instead: with
PASVADR=ANY (now the default, replacing a literal 127.0.0.1 that could only
ever be right for a local client) the client is sent to the address it
reached the control connection on. The CONFIG dump spells that out, as
does FTPD054I for the address FTPD listens on.
test/tstadr.c pins the parser down on the host and on MVS: 29 assertions,
of which 10 fail against the parsing this replaces.
NOTE for existing installations: a PARMLIB member whose SRVIP was ignored
until now takes effect with this change. SRVIP=123,0,0,1 no longer binds
every address, it binds 123.0.0.1 -- and if the stack does not have that
address, FTPD says so (FTPD051E) and does not listen.
Closes #76
Claude-Session: https://claude.ai/code/session_01Acc7qm2TkDpFjsyhwunKJk
PASVADR=ANY makes the control connection's local address the one FTPD puts in the 227 reply, so this call is now on every PASV of a stock configuration rather than only on a misconfigured one. Unchecked, a failure left the uninitialised sockaddr in place and sent the client to whatever it held. Fail the command instead -- PASV answers 425, which is the honest answer.
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
1 participant
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.
Closes #76
Was war
SRVIP,PASVADRundPASVBINDwurden auf drei Arten gelesen, mit dreiVorstellungen davon, was ein unlesbarer Wert bedeutet.
SRVIPwar dieschlechteste: der Wert wurde unverändert kopiert, die Kommaform, die die
beiden anderen akzeptieren, ließ
sscanf("%u.%u.%u.%u")eine statt vierKonversionen melden, der
else-Zweig tat nichts, unds_addrbehielt die 0aus dem
memset.SRVIP=127,0,0,1band jedes Interface des Hosts, ohne einWort — genau das Gegenteil dessen, was der Operator wollte, und genau der
Aufbau, den das Sample für den Betrieb hinter einem TLS-Proxy (#59)
empfiehlt. Ein Tippfehler (
SRVIP=127.0.0,SRVIP=localhost) war ebensostill.
Was jetzt
ftpd_adr_parse()(src/ftpd#adr.c) liest alle drei:ANY, dotted quadoder Kommaform, vier Oktette, 0-255, nichts dahinter. Nebenbei schließt das
die Löcher, die die
sscanf-Variante auch fürPASVBINDoffen ließ:999.1.2.3und1.2.3.4junkgalten als vier Konversionen, und ein zulanger Wert wurde in eine gültige andere Adresse abgeschnitten
(
192.168.100.2000->192.168.100.200). Ein abgelehnter Wert wird miteiner Meldung abgewiesen, die den einspringenden Default benennt; der
Listener startet lieber nicht, als auf
INADDR_ANYdurchzufallen.SRVBINDist der neue kanonische Name.SRVIPwird weiter gelesen,und der CONFIG-Dump schreibt
(set as SRVIP)dazu, wenn die alteSchreibweise gegriffen hat.
PASVADRbehält seinen Namen. Es ist die annoncierte Adresse, keinBind — die Vereinheitlichung der Schreibweise mit den beiden Bind-Keys
würde diesen echten Unterschied verwischen.
PASVADR=ANYist der neue Default (statt des literalen127.0.0.1,das nur für einen lokalen Client je richtig sein konnte) und heißt: die
Adresse, über die dieser Client die Kontrollverbindung erreicht hat. Damit
ist der bisher stille
getsockname()-Fallback der benannte Default statteiner dritten Fehlerbehandlung — sichtbar im CONFIG-Dump.
FTPD054IundFTPD051Enennen die Adresse, nicht nur den Port.getsockname()-RC wird geprüft (zweiter Commit): der Aufruf liegt abjetzt auf jedem PASV einer Standardkonfiguration, nicht mehr nur im
Fehlerfall. Schlägt er fehl, antwortet PASV 425, statt den Client an den
Inhalt eines uninitialisierten
sockaddrzu schicken.Test
test/tstadr.c, 29 Assertions, dual (host + MVS) —ftpd#adr.cist freivon
ftpd.hund MVS-Services. Gegen die ersetzte Parserlogik fallen davon10 um (plus ein Segfault bei
NULL); mit dem neuen Parser sindmake test-hostund der Cross-Build grün.make test-mvswurde nicht gefahren — das Modul ist gebaut, nicht auf MVSausgeführt. Getestet ist
ftpd_adr_parse()selbst; das Routing inparse_keyvalue()deckt kein Test ab.Vor dem Deploy zu erledigen
Ein PARMLIB-Member, dessen
SRVIPbisher ignoriert wurde, wird mit diesemStand wirksam. Auf dem Testsystem steht in
SYS1.PARMLIB(FTPDPRM0)SRVIP=123,0,0,1— das parst jetzt sauber, FTPD bindet 123.0.0.1, die derStack nicht hat, meldet
FTPD051E bind() failed on 123.0.0.1 port 2121undhört nicht zu. Das Member muss im selben Wartungsfenster mit umgestellt
werden (
SRVBIND=ANYoder die tatsächliche Adresse).https://claude.ai/code/session_01Acc7qm2TkDpFjsyhwunKJk