diff --git a/src/internal.c b/src/internal.c index be4ae3c25..8e3d81db7 100644 --- a/src/internal.c +++ b/src/internal.c @@ -4001,6 +4001,104 @@ static void FwdReplyRebind(WOLFSSH* ssh, const WOLFSSH_FWD_REMOTE* from, } +/* Put a request on the session's list of requests in flight, where anything + * its own send reaches can see it. */ +static void FwdPendingPush(WOLFSSH* ssh, WOLFSSH_FWD_PENDING* pend) +{ + pend->next = ssh->fwdPendingHead; + ssh->fwdPendingHead = pend; +} + + +/* Take it back off, its send being over. Requests nest, so this is not always + * the head: a callback's request commits inside the one that ran it. */ +static void FwdPendingPop(WOLFSSH* ssh, WOLFSSH_FWD_PENDING* pend) +{ + WOLFSSH_FWD_PENDING* cur; + + if (ssh->fwdPendingHead == pend) { + ssh->fwdPendingHead = pend->next; + } + else { + for (cur = ssh->fwdPendingHead; cur != NULL; cur = cur->next) { + if (cur->next == pend) { + cur->next = pend->next; + break; + } + } + } + + pend->next = NULL; +} + + +/* Forget a forward every request still in its send window held a pointer to. + * Those requests resolved it before the send; freeing it without this leaves + * their commits naming memory that is gone. */ +static void FwdPendingVoid(WOLFSSH* ssh, const WOLFSSH_FWD_REMOTE* entry) +{ + WOLFSSH_FWD_PENDING* pend; + + for (pend = ssh->fwdPendingHead; pend != NULL; pend = pend->next) { + if (pend->entry == entry) + pend->entry = NULL; + if (pend->found == entry) + pend->found = NULL; + } +} + + +/* Hand every request in flight naming one forward over to another. A request + * resolves its forward before its send, so when an answer that send pumped in + * folds two registrations into one, the survivor is what it named. */ +static void FwdPendingRebind(WOLFSSH* ssh, const WOLFSSH_FWD_REMOTE* from, + WOLFSSH_FWD_REMOTE* to) +{ + WOLFSSH_FWD_PENDING* pend; + + for (pend = ssh->fwdPendingHead; pend != NULL; pend = pend->next) { + if (pend->found == from) + pend->found = to; + } +} + + +/* A forward a request in flight is registering, or NULL. It is not on the + * session's list until that request commits, but it is what the peer is being + * asked for, so a request a callback sends meanwhile names the same one. */ +static WOLFSSH_FWD_REMOTE* FwdPendingFind(WOLFSSH* ssh, const char* bindAddr, + word32 bindPort) +{ + WOLFSSH_FWD_PENDING* pend; + + for (pend = ssh->fwdPendingHead; pend != NULL; pend = pend->next) { + if (pend->entry == NULL || pend->entry->portPending || + pend->entry->bindPort != bindPort) + continue; + if (WSTRCMP(pend->entry->bindAddr, bindAddr) == 0) + return pend->entry; + } + + return NULL; +} + + +/* Is a cancel for this forward inside its own send window? It is on the wire + * ahead of anything a callback could send from there, so it is already the + * last word on the forward. */ +static int FwdPendingHasCancel(WOLFSSH* ssh, const WOLFSSH_FWD_REMOTE* entry) +{ + WOLFSSH_FWD_PENDING* pend; + + for (pend = ssh->fwdPendingHead; pend != NULL; pend = pend->next) { + if (pend->isCancel && pend->found == entry) + return 1; + } + + return 0; +} + + static void FwdRemoteUnlink(WOLFSSH* ssh, void* heap, WOLFSSH_FWD_REMOTE* entry) { @@ -4019,6 +4117,7 @@ static void FwdRemoteUnlink(WOLFSSH* ssh, void* heap, } FwdReplyVoid(ssh, entry); + FwdPendingVoid(ssh, entry); WFREE(entry->bindAddr, heap, DYNTYPE_STRING); WFREE(entry, heap, DYNTYPE_FWD); @@ -4165,10 +4264,12 @@ static void FwdRemoteSettle(WOLFSSH* ssh, WOLFSSH_FWD_REMOTE* entry, WLOG(WS_LOG_INFO, "Remote forward reply named a port already " "registered"); /* Requests still queued on the stale entry asked about this bind, - * so they answer for the entry that stands at it now. Unlinking - * without this leaves them naming nothing, and a cancel among - * them would settle no forward. */ + * so they answer for the entry that stands at it now, and so does + * one still in its send window. Unlinking without this leaves + * them naming nothing, and a cancel among them would settle no + * forward. */ FwdReplyRebind(ssh, dup, entry); + FwdPendingRebind(ssh, dup, entry); FwdRemoteUnlink(ssh, ssh->ctx->heap, dup); } } @@ -4267,10 +4368,13 @@ int FwdRemotePrepare(WOLFSSH* ssh, const char* bindAddr, word32 bindPort, heap = ssh->ctx->heap; pend->isCancel = (byte)(isCancel != 0); - pend->bindAddr = bindAddr; - pend->bindPort = bindPort; found = FwdRemoteFind(ssh, bindAddr, bindPort); + /* A request a send callback is making names the forward the request that + * ran it is registering, which is on the wire but not on the list yet. */ + if (found == NULL) + found = FwdPendingFind(ssh, bindAddr, bindPort); + if (isCancel) { if (found == NULL) { WOLFSSH_FWD_REMOTE* cur; @@ -4339,6 +4443,13 @@ int FwdRemotePrepare(WOLFSSH* ssh, const char* bindAddr, word32 bindPort, /* An error leaves nothing to commit and nothing to give back. */ if (ret != WS_SUCCESS) WMEMSET(pend, 0, sizeof(*pend)); + else { + /* What this request resolved to is settled here rather than looked up + * again at commit: by then a callback the send ran may have registered + * the same bind anew, and this request went out ahead of it. */ + pend->found = found; + FwdPendingPush(ssh, pend); + } WLOG(WS_LOG_DEBUG, "Leaving FwdRemotePrepare(), ret = %d", ret); return ret; @@ -4370,18 +4481,23 @@ int FwdReplyPrepare(WOLFSSH* ssh, WOLFSSH_FWD_PENDING* pend) pend->reply = FwdReplyNew(ssh, 0, NULL, 0); ret = pend->reply == NULL ? WS_MEMORY_E : WS_SUCCESS; + /* It names no forward, but every request in flight is on the list. */ + if (ret == WS_SUCCESS) + FwdPendingPush(ssh, pend); + WLOG(WS_LOG_DEBUG, "Leaving FwdReplyPrepare(), ret = %d", ret); return ret; } -/* The request reached the wire, so link what was prepared. The registration it - * names is looked up again here: the send runs the application's send and - * highwater callbacks, which can reenter the library and free the entry a - * pointer held across the send would name. */ +/* The request reached the wire, so link what was prepared. The forward it names + * was resolved before the send and held on the pending: the send runs the + * application's send and highwater callbacks, which can reenter the library, + * and a lookup from here would find what those did afterwards. */ void FwdPendingCommit(WOLFSSH* ssh, WOLFSSH_FWD_PENDING* pend) { - WOLFSSH_FWD_REMOTE* target = NULL; + WOLFSSH_FWD_REMOTE* target; + WOLFSSH_FWD_REMOTE* dup; WOLFSSH_FWD_REMOTE* cur; void* heap; @@ -4392,15 +4508,26 @@ void FwdPendingCommit(WOLFSSH* ssh, WOLFSSH_FWD_PENDING* pend) heap = ssh->ctx->heap; - if (pend->bindAddr != NULL) - target = FwdRemoteFind(ssh, pend->bindAddr, pend->bindPort); + /* Its send is over, so nothing a later request sends can name it. */ + FwdPendingPop(ssh, pend); - if (pend->entry != NULL && target != NULL) { - /* A callback the send ran registered this bind first, so the entry - * built for it is one too many. */ - WFREE(pend->entry->bindAddr, heap, DYNTYPE_STRING); - WFREE(pend->entry, heap, DYNTYPE_FWD); - pend->entry = NULL; + target = pend->entry != NULL ? pend->entry : pend->found; + + /* An answer the send pumped in named the port this request asks for, so + * the bind stands registered already and the entry built for it is one too + * many. A cancel names a forward by its bind alone, so a bind gets one + * registration. A port-0 request has no port to be found by, and folds + * when the answer to it names one. */ + if (pend->entry != NULL && !pend->entry->portPending) { + dup = FwdRemoteFind(ssh, pend->entry->bindAddr, pend->entry->bindPort); + if (dup != NULL) { + /* A request a callback sent named the entry built here, so it + * answers for the one that stands at the bind now. */ + FwdReplyRebind(ssh, pend->entry, dup); + FwdRemoteUnlink(ssh, heap, pend->entry); + pend->entry = NULL; + target = dup; + } } if (pend->entry != NULL) { @@ -4412,12 +4539,6 @@ void FwdPendingCommit(WOLFSSH* ssh, WOLFSSH_FWD_PENDING* pend) ssh->fwdRemoteList = pend->entry; else cur->next = pend->entry; - - /* From here on, forwarded-tcpip opens are matched against this list. A - * client that never calls wolfSSH_FwdRemoteSetup() never sets this and - * has its opens go unchecked. */ - ssh->fwdRemoteTracked = 1; - target = pend->entry; } if (pend->reply != NULL && pend->reply->answered) { @@ -4467,9 +4588,13 @@ void FwdPendingDiscard(WOLFSSH* ssh, WOLFSSH_FWD_PENDING* pend) heap = ssh->ctx->heap; + /* Its send is over, so nothing a later request sends can name it. */ + FwdPendingPop(ssh, pend); + if (pend->entry != NULL) { - WFREE(pend->entry->bindAddr, heap, DYNTYPE_STRING); - WFREE(pend->entry, heap, DYNTYPE_FWD); + /* Nothing linked it, so there is no list to take it out of, but a + * request a callback sent may have named it and has to let go. */ + FwdRemoteUnlink(ssh, heap, pend->entry); } if (pend->reply != NULL) { /* An answer that arrived mid-send dequeued the slot already, and @@ -4486,10 +4611,26 @@ void FwdPendingDiscard(WOLFSSH* ssh, WOLFSSH_FWD_PENDING* pend) } +/* Does the bind this open names reach that forward? A peer that rewrites the + * bind it echoes back can still be held to the port it was asked for. */ +static int FwdRemoteAddrMatch(WOLFSSH* ssh, const WOLFSSH_FWD_REMOTE* entry, + const char* addr, word32 port) +{ + /* No port to match on until the peer's reply names the one it bound. */ + if (entry->portPending || entry->bindPort != port) + return 0; + + return ssh->fwdRemoteMatch == WOLFSSH_FWD_MATCH_PORT || + FwdRemoteAddrIsWild(entry->bindAddr) || + WSTRCMP(entry->bindAddr, addr) == 0; +} + + /* Does an inbound forwarded-tcpip name a forward this client registered? */ static int FwdRemoteMatch(WOLFSSH* ssh, const char* addr, word32 port) { WOLFSSH_FWD_REMOTE* cur; + WOLFSSH_FWD_PENDING* pend; if (ssh == NULL || addr == NULL) return 0; @@ -4501,16 +4642,16 @@ static int FwdRemoteMatch(WOLFSSH* ssh, const char* addr, word32 port) for (cur = ssh->fwdRemoteList; cur != NULL; cur = cur->next) { WOLFSSH_FWD_REPLY* newest; - /* No port to match on until the peer's reply names the one it - * bound. */ - if (cur->portPending || cur->bindPort != port) + if (!FwdRemoteAddrMatch(ssh, cur, addr, port)) continue; /* The newest request governs: a cancel stops matching as it goes out, * so revoking never waits on the peer, and the peer refusing it puts - * the forward back. */ + * the forward back. A cancel still inside its own send counts, since + * the session can only be pumped from a callback that send ran. */ newest = FwdReplyNewest(ssh, cur); - if (newest != NULL && newest->isCancel) + if ((newest != NULL && newest->isCancel) || + FwdPendingHasCancel(ssh, cur)) continue; /* A forward stands on the peer having bound it, or on a request still @@ -4518,12 +4659,36 @@ static int FwdRemoteMatch(WOLFSSH* ssh, const char* addr, word32 port) if (!cur->confirmed && newest == NULL) continue; - /* A peer that rewrites the bind it echoes back can still be held to - * the port it was asked for. */ - if (ssh->fwdRemoteMatch == WOLFSSH_FWD_MATCH_PORT || - FwdRemoteAddrIsWild(cur->bindAddr) || - WSTRCMP(cur->bindAddr, addr) == 0) - return 1; + return 1; + } + + /* A setup inside its own send is already on the wire, so the listener it + * asks for can start feeding channels before the call returns. It is the + * newest request for its bind by construction, and needs no reply to speak + * for it. */ + for (pend = ssh->fwdPendingHead; pend != NULL; pend = pend->next) { + WOLFSSH_FWD_REPLY* newest; + + if (pend->entry == NULL || + !FwdRemoteAddrMatch(ssh, pend->entry, addr, port)) + continue; + + /* A cancel a callback sent from this setup's send went out behind it + * and committed, so the newest request governs here too. */ + newest = FwdReplyNewest(ssh, pend->entry); + if ((newest != NULL && newest->isCancel) || + FwdPendingHasCancel(ssh, pend->entry)) + continue; + + /* An answer that arrived mid-send left the queue for the commit to + * apply, so no scan of it sees this request. The peer refused the + * bind, so nothing speaks for the forward and the commit is about to + * drop it. */ + if (pend->reply != NULL && pend->reply->answered && + !pend->reply->success) + continue; + + return 1; } return 0; @@ -11810,10 +11975,12 @@ static int DoChannelOpen(WOLFSSH* ssh, /* Per RFC 4254 7.2, a forwarded-tcpip open answers a forward the * client registered with tcpip-forward, so refuse one naming - * anything else before the policy callback sees it. Only a client - * that used wolfSSH_FwdRemoteSetup() has a list to check. */ + * anything else before the policy callback sees it. A client that + * registered nothing has nothing an open can answer for, which is + * why an empty list refuses rather than admits. An application + * keeping its own list can say so with + * wolfSSH_SetFwdRemoteMatch(). */ if (ret == WS_SUCCESS && typeId == ID_CHANTYPE_TCPIP_FORWARD && - ssh->fwdRemoteTracked && !FwdRemoteMatch(ssh, host, hostPort)) { WLOG(WS_LOG_WARN, "Rejecting forwarded-tcpip channel open " "for the unregistered forward %s:%u", diff --git a/tests/regress.c b/tests/regress.c index 079d581d2..dc0d3bd25 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -3885,7 +3885,10 @@ static void TestForwardedTcpipRefusedForwardSendsOpenFail(void) FreeChannelOpenHarness(&harness); } -static void TestForwardedTcpipUntrackedClientUnchanged(void) +/* A client that asked for no forwards has nothing an open could answer for, so + * an empty list refuses every one of them. The fwdCb accepts throughout: it is + * the library that refuses, not the application. */ +static void TestForwardedTcpipNoForwardsRefusesOpen(void) { ChannelOpenHarness harness; byte extra[128]; @@ -3903,8 +3906,36 @@ static void TestForwardedTcpipUntrackedClientUnchanged(void) AssertIntEQ(wolfSSH_CTX_SetFwdCb(harness.ctx, AcceptFwdCb, NULL), WS_SUCCESS); - /* A client that frames tcpip-forward itself registers nothing, so there - * is no list to match against and its fwdCb stays the only gate. */ + ret = DoReceive(harness.ssh); + AssertChannelOpenFailResponse(&harness, ret); + AssertIntEQ(ParseChannelOpenFailReason(harness.io.out, harness.io.outSz), + OPEN_ADMINISTRATIVELY_PROHIBITED); + + FreeChannelOpenHarness(&harness); +} + +/* The opt-out puts it back the way it was: an application keeping its own + * bind list answers for what it accepts. */ +static void TestForwardedTcpipMatchOffNoForwardsAccepts(void) +{ + ChannelOpenHarness harness; + byte extra[128]; + byte in[192]; + word32 extraSz; + word32 inSz; + int ret; + + extraSz = BuildDirectTcpipExtra("10.0.0.1", 9999, "10.0.0.5", 4321, + extra, sizeof(extra)); + inSz = BuildChannelOpenPacket("forwarded-tcpip", 9, 0x4000, 0x8000, + extra, extraSz, in, sizeof(in)); + + InitChannelOpenHarnessClient(&harness, in, inSz); + AssertIntEQ(wolfSSH_CTX_SetFwdCb(harness.ctx, AcceptFwdCb, NULL), + WS_SUCCESS); + AssertIntEQ(wolfSSH_SetFwdRemoteMatch(harness.ssh, WOLFSSH_FWD_MATCH_OFF), + WS_SUCCESS); + ret = DoReceive(harness.ssh); AssertIntEQ(ret, WS_SUCCESS); AssertTrue(harness.io.outSz > 0); @@ -4371,7 +4402,6 @@ static void RunForwardedTcpipPostSendErrorTest(WS_CallbackHighwater cb, AssertIntEQ(wolfSSH_FwdRemoteSetup(harness.ssh, "127.0.0.1", 8080, 0), expectRet); AssertNotNull(harness.ssh->fwdRemoteList); - AssertIntEQ(harness.ssh->fwdRemoteTracked, 1); harness.io.outSz = 0; @@ -4411,7 +4441,6 @@ static void TestForwardedTcpipFailedSendRegistersNothing(void) AssertTrue(harness.ssh->fwdRemoteList == NULL); AssertTrue(harness.ssh->fwdReplyHead == NULL); AssertTrue(harness.ssh->fwdReplyTail == NULL); - AssertIntEQ(harness.ssh->fwdRemoteTracked, 0); FreeChannelOpenHarness(&harness); } @@ -4719,10 +4748,11 @@ static int FwdReplyFromSendCb(WOLFSSH* ssh, void* buf, word32 sz, void* ctx) return ret; } -static void InitReplyFromSendHarness(ChannelOpenHarness* harness, byte msgId, +/* Arm the callback on a harness in use, for a test that has to get the session + * somewhere before the answer lands. */ +static void ArmReplyFromSend(ChannelOpenHarness* harness, byte msgId, const byte* data, word32 dataSz) { - InitFwdRemoteHarness(harness); wolfSSH_SetIOSend(harness->ctx, FwdReplyFromSendCb); WMEMSET(&fwdReplyFromSend, 0, sizeof(fwdReplyFromSend)); @@ -4732,6 +4762,13 @@ static void InitReplyFromSendHarness(ChannelOpenHarness* harness, byte msgId, fwdReplyFromSend.dataSz = dataSz; } +static void InitReplyFromSendHarness(ChannelOpenHarness* harness, byte msgId, + const byte* data, word32 dataSz) +{ + InitFwdRemoteHarness(harness); + ArmReplyFromSend(harness, msgId, data, dataSz); +} + /* The answer arrives with the slot still uncommitted, so its verdict parks * there for the commit to apply. The forward stands confirmed all the same. */ static void TestForwardedTcpipReplyFromSendCommits(void) @@ -4806,6 +4843,96 @@ static void TestForwardedTcpipPortZeroReplyFromSendBinds(void) FreeChannelOpenHarness(&harness); } +/* A port-0 forward the peer resolves to the port a setup in flight is asking + * for. The fold at settle scans the session's list, which that setup hasn't + * joined yet, so its commit is the one place the two can be folded. Without + * that, one bind takes two registrations and a cancel drops only the first. */ +static void TestForwardedTcpipCommitFoldsSettledDuplicate(void) +{ + ChannelOpenHarness harness; + byte port[UINT32_SZ]; + word32 portSz; + + InitFwdRemoteHarness(&harness); + + /* A port-0 setup, still owed the port the peer bound. */ + AssertIntEQ(wolfSSH_FwdRemoteSetup(harness.ssh, "127.0.0.1", 0, 1), + WS_SUCCESS); + AssertIntEQ(FwdRemoteCount(harness.ssh), 1); + + /* The next setup's send pumps in that answer, naming the port it is + * itself registering. */ + portSz = AppendUint32(port, sizeof(port), 0, 8080); + ArmReplyFromSend(&harness, MSGID_REQUEST_SUCCESS, port, portSz); + + AssertIntEQ(wolfSSH_FwdRemoteSetup(harness.ssh, "127.0.0.1", 8080, 1), + WS_SUCCESS); + AssertIntEQ(FwdRemoteCount(harness.ssh), 1); + AssertIntEQ(harness.ssh->fwdRemoteList->bindPort, 8080); + + /* One registration, so one cancel revokes the bind. */ + AssertIntEQ(wolfSSH_FwdRemoteCancel(harness.ssh, "127.0.0.1", 8080, 1), + WS_SUCCESS); + harness.io.outSz = 0; + AssertForwardedOpenRefused(&harness, "127.0.0.1", 8080); + + FreeChannelOpenHarness(&harness); +} + +/* The mirror of that: an answer the cancel's send pumps in folds the very + * registration the cancel resolved to on the way in, and the unlink has to + * hand the cancel over rather than void it. Otherwise the cancel commits + * naming nothing and the registration standing at the bind is never + * revoked. */ +static void TestForwardedTcpipSettleFoldRebindsSendingCancel(void) +{ + ChannelOpenHarness harness; + byte port[UINT32_SZ]; + word32 portSz; + + InitFwdRemoteHarness(&harness); + + /* A port-0 setup, still owed the port the peer bound. */ + AssertIntEQ(wolfSSH_FwdRemoteSetup(harness.ssh, "127.0.0.1", 0, 1), + WS_SUCCESS); + AssertIntEQ(FwdRemoteCount(harness.ssh), 1); + + /* An explicit-port registration for the cancel to resolve to. */ + harness.io.outSz = 0; + AssertIntEQ(wolfSSH_FwdRemoteSetup(harness.ssh, "127.0.0.1", 8080, 0), + WS_SUCCESS); + AssertIntEQ(FwdRemoteCount(harness.ssh), 2); + + /* The cancel's send pumps in the port-0 answer, naming the port the + * cancel is revoking, which folds the entry the cancel resolved to. */ + portSz = AppendUint32(port, sizeof(port), 0, 8080); + ArmReplyFromSend(&harness, MSGID_REQUEST_SUCCESS, port, portSz); + + harness.io.outSz = 0; + AssertIntEQ(wolfSSH_FwdRemoteCancel(harness.ssh, "127.0.0.1", 8080, 1), + WS_SUCCESS); + + /* One registration, and the cancel names it. */ + AssertIntEQ(FwdRemoteCount(harness.ssh), 1); + AssertIntEQ(harness.ssh->fwdRemoteList->bindPort, 8080); + AssertNotNull(harness.ssh->fwdReplyHead); + AssertIntEQ(harness.ssh->fwdReplyHead->isCancel, 1); + AssertTrue(harness.ssh->fwdReplyHead->entry == harness.ssh->fwdRemoteList); + + /* A cancel stops matching as it goes out. */ + harness.io.outSz = 0; + AssertForwardedOpenRefused(&harness, "127.0.0.1", 8080); + + /* The peer took the listener down, and nothing is left at the bind. */ + harness.io.outSz = 0; + FeedRequestSuccess(&harness); + AssertIntEQ(FwdRemoteCount(harness.ssh), 0); + AssertTrue(harness.ssh->fwdReplyHead == NULL); + + FreeChannelOpenHarness(&harness); +} + + /* The peer answered what it had seen of the request, then the rest of the * send failed. The request never reached the peer whole, so the parked * verdict settles nothing and goes back with the slot. */ @@ -4826,7 +4953,6 @@ static void TestForwardedTcpipFailedSendDiscardsAnsweredSlot(void) AssertTrue(harness.ssh->fwdReplyHead == NULL); AssertTrue(harness.ssh->fwdReplyTail == NULL); AssertIntEQ(harness.ssh->fwdReplyCount, 0); - AssertIntEQ(harness.ssh->fwdRemoteTracked, 0); FreeChannelOpenHarness(&harness); } @@ -5040,10 +5166,7 @@ static int InboundOpenDuringSendHighwaterCb(byte side, void* ctx) WOLFSSH_UNUSED(side); - /* The request is on the wire, so matching has to be live already: this is - * the first setup, and until it registers nothing is tracked and every - * open goes unchecked. */ - AssertIntEQ(harness->ssh->fwdRemoteTracked, 1); + /* The request is on the wire, so matching has to be live already. */ AssertForwardedOpenRefused(harness, "10.0.0.1", 9999); return WS_SUCCESS; @@ -5071,6 +5194,293 @@ static void TestForwardedTcpipInboundOpenDuringSend(void) FreeChannelOpenHarness(&harness); } +/* The highwater callback runs after the request has committed, so whatever it + * does lands on settled state. The IO send callback is the one place a + * forwarding call runs with the request that ran it still framed and + * uncommitted: the peer has the bytes, and this side has yet to write anything + * down. The tests below drive one from there. + * + * Sending from inside a send flushes the buffer the outer send is partway + * through, so these report a would-block rather than count the same bytes out + * twice. The outer request still reached the peer, which is what + * SendPacketDelivered() reports and what these turn on. */ +enum { + FWD_REENTER_CANCEL = 1, /* cancel addr:port from the send */ + FWD_REENTER_SETUP, /* register addr:port from the send */ + FWD_REENTER_OPEN, /* feed a forwarded-tcpip for addr:port */ + FWD_REENTER_CANCEL_OPEN, /* cancel addr:port asking for a reply, then + * feed a forwarded-tcpip for it */ + FWD_REENTER_FAIL_OPEN /* answer the request with a REQUEST_FAILURE, + * then feed a forwarded-tcpip for it */ +}; + +typedef struct { + ChannelOpenHarness* harness; + const char* addr; + word32 port; + int action; + int calls; + int ret; /* what the reentrant call returned */ + int openMsgId; /* how an open fed from the send was answered */ + word32 openChannels;/* channels the session held right after it */ +} FwdReentrantFromSend; + +static FwdReentrantFromSend fwdReentrantFromSend; + +static int FwdReentrantFromSendCb(WOLFSSH* ssh, void* buf, word32 sz, void* ctx) +{ + FwdReentrantFromSend* state = &fwdReentrantFromSend; + byte in[192]; + word32 inSz; + + state->calls++; + + /* The reentrant call's own sends, this one's bytes among them. */ + if (state->calls > 1) + return MemSend(ssh, buf, sz, ctx); + + switch (state->action) { + case FWD_REENTER_CANCEL: + state->ret = wolfSSH_FwdRemoteCancel(ssh, state->addr, state->port, + 0); + break; + + case FWD_REENTER_SETUP: + state->ret = wolfSSH_FwdRemoteSetup(ssh, state->addr, state->port, + 0); + break; + + case FWD_REENTER_OPEN: + inSz = BuildForwardedTcpipOpen(state->addr, state->port, in, + sizeof(in)); + state->ret = FeedOnePacket(state->harness, in, inSz); + break; + + case FWD_REENTER_FAIL_OPEN: + FeedRequestFailure(state->harness); + inSz = BuildForwardedTcpipOpen(state->addr, state->port, in, + sizeof(in)); + state->ret = FeedOnePacket(state->harness, in, inSz); + state->openChannels = state->harness->ssh->channelListSz; + break; + + case FWD_REENTER_CANCEL_OPEN: + state->ret = wolfSSH_FwdRemoteCancel(ssh, state->addr, + state->port, 1); + inSz = BuildForwardedTcpipOpen(state->addr, state->port, in, + sizeof(in)); + (void)FeedOnePacket(state->harness, in, inSz); + state->openMsgId = ParseMsgId(state->harness->io.out, + state->harness->io.outSz); + state->openChannels = state->harness->ssh->channelListSz; + break; + } + + return WS_CBIO_ERR_WANT_WRITE; +} + +static void ArmFwdReentrantFromSend(ChannelOpenHarness* harness, int action, + const char* addr, word32 port) +{ + wolfSSH_SetIOSend(harness->ctx, FwdReentrantFromSendCb); + + WMEMSET(&fwdReentrantFromSend, 0, sizeof(fwdReentrantFromSend)); + fwdReentrantFromSend.harness = harness; + fwdReentrantFromSend.action = action; + fwdReentrantFromSend.addr = addr; + fwdReentrantFromSend.port = port; +} + +/* Where the second packet in the output starts. What the callback sent or + * answered went out behind the request that was still in the buffer. */ +static word32 NextPacketOffset(const byte* out, word32 outSz) +{ + word32 packetLen; + + AssertTrue(outSz > UINT32_SZ); + WMEMCPY(&packetLen, out, sizeof(packetLen)); + packetLen = ntohl(packetLen); + + AssertTrue(outSz > UINT32_SZ + packetLen); + return UINT32_SZ + packetLen; +} + +/* A cancel sent from inside a first setup's send names a forward that exists + * only on the request in flight. It went out behind that setup, so the peer + * holds no listener and neither may this side -- and the setup's commit must + * not put back what the cancel took. */ +static void TestForwardedTcpipCancelFromSendDropsForward(void) +{ + ChannelOpenHarness harness; + + InitFwdRemoteHarness(&harness); + ArmFwdReentrantFromSend(&harness, FWD_REENTER_CANCEL, "127.0.0.1", 8080); + + AssertIntEQ(wolfSSH_FwdRemoteSetup(harness.ssh, "127.0.0.1", 8080, 1), + WS_WANT_WRITE); + AssertIntEQ(fwdReentrantFromSend.ret, WS_SUCCESS); + + /* Both requests reached the peer, the cancel second. */ + AssertIntEQ(FwdRemoteCount(harness.ssh), 0); + + /* The setup asked for a reply and the peer owes one, so the slot stays + * queued. It answers for no forward now. */ + AssertIntEQ(harness.ssh->fwdReplyCount, 1); + + harness.io.outSz = 0; + FeedRequestSuccess(&harness); + AssertIntEQ(harness.ssh->fwdReplyCount, 0); + AssertIntEQ(FwdRemoteCount(harness.ssh), 0); + + harness.io.outSz = 0; + AssertForwardedOpenRefused(&harness, "127.0.0.1", 8080); + + FreeChannelOpenHarness(&harness); +} + +/* The same window the other way: a setup sent from inside an unmatched + * cancel's send. The setup went out last, so its registration stands, and the + * answer the cancel is owed speaks for a request that named nothing. */ +static void TestForwardedTcpipSetupFromSendSurvivesCancelReply(void) +{ + ChannelOpenHarness harness; + + InitFwdRemoteHarness(&harness); + ArmFwdReentrantFromSend(&harness, FWD_REENTER_SETUP, "127.0.0.1", 8080); + + AssertIntEQ(wolfSSH_FwdRemoteCancel(harness.ssh, "127.0.0.1", 8080, 1), + WS_WANT_WRITE); + AssertIntEQ(fwdReentrantFromSend.ret, WS_SUCCESS); + + AssertIntEQ(FwdRemoteCount(harness.ssh), 1); + AssertIntEQ(harness.ssh->fwdRemoteList->confirmed, 1); + + /* The cancel's slot answers for nothing, so a success on it takes no + * listener down. */ + AssertIntEQ(harness.ssh->fwdReplyCount, 1); + AssertTrue(harness.ssh->fwdReplyHead->entry == NULL); + + harness.io.outSz = 0; + FeedRequestSuccess(&harness); + AssertIntEQ(FwdRemoteCount(harness.ssh), 1); + + harness.io.outSz = 0; + AssertForwardedOpenRefused(&harness, "10.0.0.1", 9999); + AssertForwardedOpenAccepted(&harness, "127.0.0.1", 8080, 1); + + FreeChannelOpenHarness(&harness); +} + +/* Pump an inbound forwarded-tcpip open from inside the send of a request for + * bindAddr:bindPort. A cancel needs a forward to name, so that case starts + * from a settled registration. */ +static void RunFwdOpenDuringSendTest(int isCancel, const char* bindAddr, + word32 bindPort, const char* openAddr, word32 openPort, + int expectAccept) +{ + ChannelOpenHarness harness; + word32 off; + + InitFwdRemoteHarness(&harness); + + if (isCancel) { + AssertIntEQ(wolfSSH_FwdRemoteSetup(harness.ssh, bindAddr, bindPort, 0), + WS_SUCCESS); + AssertIntEQ(FwdRemoteCount(harness.ssh), 1); + } + + ArmFwdReentrantFromSend(&harness, FWD_REENTER_OPEN, openAddr, openPort); + + if (isCancel) + AssertIntEQ(wolfSSH_FwdRemoteCancel(harness.ssh, bindAddr, bindPort, 0), + WS_WANT_WRITE); + else + AssertIntEQ(wolfSSH_FwdRemoteSetup(harness.ssh, bindAddr, bindPort, 1), + WS_WANT_WRITE); + + AssertIntEQ(fwdReentrantFromSend.ret, WS_SUCCESS); + + off = NextPacketOffset(harness.io.out, harness.io.outSz); + AssertIntEQ(ParseMsgId(harness.io.out + off, harness.io.outSz - off), + expectAccept ? MSGID_CHANNEL_OPEN_CONF : MSGID_CHANNEL_OPEN_FAIL); + AssertIntEQ(harness.ssh->channelListSz, expectAccept ? 1 : 0); + + FreeChannelOpenHarness(&harness); +} + +/* The request is on the wire before the call returns, so the listener it asks + * for can start feeding channels from there. Nothing on the session's list + * names that bind yet, and the reply slot does not name it until the commit. */ +static void TestForwardedTcpipOpenDuringSetupSendAccepted(void) +{ + RunFwdOpenDuringSendTest(0, "127.0.0.1", 8080, "127.0.0.1", 8080, 1); +} + +/* Live matching, not a check that stopped running: a bind this side never + * asked for is refused in the same window. */ +static void TestForwardedTcpipOpenDuringSetupSendRefusesOther(void) +{ + RunFwdOpenDuringSendTest(0, "127.0.0.1", 8080, "10.0.0.1", 9999, 0); +} + +/* A cancel with no reply asked for is the last word on its forward as it goes + * out, so an open naming that bind is refused from inside the cancel's own + * send. */ +static void TestForwardedTcpipOpenDuringCancelSendRefused(void) +{ + RunFwdOpenDuringSendTest(1, "127.0.0.1", 8080, "127.0.0.1", 8080, 0); +} + +/* A cancel that asks for a reply commits inside the setup's send and leaves + * the list of requests in flight, so only its queued slot still names the + * forward. It went out behind the setup either way, so an open naming that + * bind is refused from inside the same send. */ +static void TestForwardedTcpipOpenAfterCancelWithReplyRefused(void) +{ + ChannelOpenHarness harness; + + InitFwdRemoteHarness(&harness); + ArmFwdReentrantFromSend(&harness, FWD_REENTER_CANCEL_OPEN, + "127.0.0.1", 8080); + + AssertIntEQ(wolfSSH_FwdRemoteSetup(harness.ssh, "127.0.0.1", 8080, 1), + WS_WANT_WRITE); + AssertIntEQ(fwdReentrantFromSend.ret, WS_SUCCESS); + + AssertIntEQ(fwdReentrantFromSend.openMsgId, MSGID_CHANNEL_OPEN_FAIL); + AssertIntEQ(fwdReentrantFromSend.openChannels, 0); + + FreeChannelOpenHarness(&harness); +} + +/* The peer refuses the setup while it is still being sent. That answer leaves + * the reply queue for the commit to apply, so no scan of the queue sees it, + * and the request itself is the only thing left saying the bind was ever + * asked for. An open naming it has to be refused: the commit is about to drop + * the forward, and a channel admitted here would outlive it. */ +static void TestForwardedTcpipOpenAfterMidSendFailureRefused(void) +{ + ChannelOpenHarness harness; + + InitFwdRemoteHarness(&harness); + ArmFwdReentrantFromSend(&harness, FWD_REENTER_FAIL_OPEN, + "127.0.0.1", 8080); + + AssertIntEQ(wolfSSH_FwdRemoteSetup(harness.ssh, "127.0.0.1", 8080, 1), + WS_WANT_WRITE); + + AssertIntEQ(fwdReentrantFromSend.ret, WS_SUCCESS); + AssertIntEQ(fwdReentrantFromSend.openChannels, 0); + + /* The refusal settled at the commit, so the forward is gone and the + * queue is clear. */ + AssertIntEQ(FwdRemoteCount(harness.ssh), 0); + AssertIntEQ(harness.ssh->fwdReplyCount, 0); + AssertIntEQ(harness.ssh->channelListSz, 0); + + FreeChannelOpenHarness(&harness); +} + static void TestFwdRemoteMatchPortIgnoresBindAddr(void) { RunForwardedTcpipMatchModeTest(WOLFSSH_FWD_MATCH_STRICT, "localhost", 8080, @@ -10863,7 +11273,8 @@ int main(int argc, char** argv) TestForwardedTcpipCancelledSendsOpenFail(); TestForwardedTcpipPortZeroMatchesBoundPort(); TestForwardedTcpipRefusedForwardSendsOpenFail(); - TestForwardedTcpipUntrackedClientUnchanged(); + TestForwardedTcpipNoForwardsRefusesOpen(); + TestForwardedTcpipMatchOffNoForwardsAccepts(); TestForwardedTcpipCancelConfirmedSendsOpenFail(); TestForwardedTcpipCancelPendingStopsMatching(); TestForwardedTcpipCancelRefusedRestoresMatching(); @@ -10893,6 +11304,8 @@ int main(int argc, char** argv) TestForwardedTcpipReplyFromSendCommits(); TestForwardedTcpipRefusalFromSendDropsForward(); TestForwardedTcpipPortZeroReplyFromSendBinds(); + TestForwardedTcpipCommitFoldsSettledDuplicate(); + TestForwardedTcpipSettleFoldRebindsSendingCancel(); TestForwardedTcpipFailedSendDiscardsAnsweredSlot(); TestForwardedTcpipRefusalFromSendSeesSendingCancel(); TestForwardedTcpipRefusalDuringSendDropsForward(); @@ -10903,6 +11316,13 @@ int main(int argc, char** argv) TestForwardedTcpipReentrantCancelOfFirstSetup(); TestForwardedTcpipReentrantSetupDuringCancel(); TestForwardedTcpipInboundOpenDuringSend(); + TestForwardedTcpipCancelFromSendDropsForward(); + TestForwardedTcpipSetupFromSendSurvivesCancelReply(); + TestForwardedTcpipOpenDuringSetupSendAccepted(); + TestForwardedTcpipOpenDuringSetupSendRefusesOther(); + TestForwardedTcpipOpenDuringCancelSendRefused(); + TestForwardedTcpipOpenAfterCancelWithReplyRefused(); + TestForwardedTcpipOpenAfterMidSendFailureRefused(); TestFwdRemoteMatchPortIgnoresBindAddr(); TestFwdRemoteMatchOffAcceptsUnregistered(); TestFwdRemoteMatchRejectsBadSetting(); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 2201d4caa..2dd9b0636 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1060,14 +1060,24 @@ typedef struct WOLFSSH_FWD_REPLY { /* Bookkeeping for a global request that has not been sent yet. Everything that * can fail is allocated into one of these first, so a caller that gets an error - * back knows nothing reached the peer. The registration is named by its bind - * rather than by a pointer, since sending runs application callbacks that may - * reenter the library and free it. */ + * back knows nothing reached the peer. + * + * Sending runs application callbacks that may reenter the library and send + * requests of their own, so one of these is on the session's list of requests + * in flight for the length of its send. That is what lets a reentrant request + * see the forward the request it interrupted is registering, and what keeps a + * request from resolving to a registration made after it went out. Nothing on + * the list outlives its send: the frame that owns it cannot return until every + * call it made has. */ typedef struct WOLFSSH_FWD_PENDING { + struct WOLFSSH_FWD_PENDING* next; /* next request in flight */ WOLFSSH_FWD_REMOTE* entry; /* new registration to link on commit */ + WOLFSSH_FWD_REMOTE* found; /* registration this request named, if it was + * already there. Cleared if it is freed + * mid-send, so a commit never settles a + * registration that is gone or one that + * replaced it. */ WOLFSSH_FWD_REPLY* reply; /* reply slot to queue on commit */ - const char* bindAddr; /* bind the request names, NULL if none */ - word32 bindPort; byte isCancel; } WOLFSSH_FWD_PENDING; @@ -1308,8 +1318,8 @@ struct WOLFSSH { WOLFSSH_FWD_REMOTE* fwdRemoteList; /* remote forwards this client asked for */ WOLFSSH_FWD_REPLY* fwdReplyHead; /* oldest want-reply request owed */ WOLFSSH_FWD_REPLY* fwdReplyTail; + WOLFSSH_FWD_PENDING* fwdPendingHead; /* requests inside their own send */ word32 fwdReplyCount; /* slots queued, kept off the walk it bounds */ - byte fwdRemoteTracked; /* wolfSSH_FwdRemoteSetup() was used */ byte fwdRemoteMatch; /* WOLFSSH_FWD_MATCH_*, how strictly an inbound * forwarded-tcpip must name a registration */ #endif /* WOLFSSH_FWD */ diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index ec1737471..417bfc059 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -302,13 +302,14 @@ DEPRECATED WOLFSSH_API int wolfSSH_ChannelGetFwdFd( * the peer to choose, and needs wantReply, since its reply is the only place * the bound port is named; without it the call returns WS_BAD_ARGUMENT. * - * From the first call on, this session refuses any "forwarded-tcpip" open - * naming a bind it did not register, per RFC 4254 7.2. A bind of "", "*", - * "0.0.0.0", or an IPv6 any-address matches on port alone; anything else must - * equal the address the peer reports. Register the spelling the peer will echo - * back, or a wildcard: a peer that canonicalises the bind, answering an open - * for "127.0.0.1" against a registered "localhost", has those opens refused. - * wolfSSH_SetFwdRemoteMatch() relaxes this for peers that need it. + * A client refuses any "forwarded-tcpip" open naming a bind it has not + * registered, per RFC 4254 7.2, from the session's start rather than from the + * first call here, so one that registers nothing refuses them all. A bind of + * "", "*", "0.0.0.0", or an IPv6 any-address matches on port alone; anything + * else must equal the address the peer reports. Register the spelling the peer + * will echo back, or a wildcard: a peer that canonicalises the bind, answering + * an open for "127.0.0.1" against a registered "localhost", has those opens + * refused. wolfSSH_SetFwdRemoteMatch() relaxes this for peers that need it. * * One bindAddr:bindPort is one registration however often it is registered, * since it is one listener on the peer, so one cancel undoes it. That covers a @@ -350,8 +351,9 @@ enum WS_FwdRemoteMatch { WOLFSSH_FWD_MATCH_OFF = 2 /* accept any open, matching nothing */ }; -/* Relax the check wolfSSH_FwdRemoteSetup() turns on for this session. Set it - * before the first setup, since opens are matched from that point. +/* Relax the match this session holds inbound "forwarded-tcpip" opens to. A + * client matches them from the start, so a session that registered nothing + * refuses them all; set this before the peer can send one. * * STRICT is the default and is what RFC 4254 7.2 asks for. PORT is for a peer * that rewrites the bind address it echoes back but keeps the port, which