Skip to content

Commit 32038f1

Browse files
tomaswolftomaswolf
committed
GH-861: prevent zero-byte write in SFTP
In certain circumstances SftpOutputStreamAsync.flush() could cause sending an SSH_FXP_WRITE with zero data bytes. While this is not forbidden in SFTP, it is unnecessary and there is at least one SFTP server that (erroneously) rejects such zero-byte writes with an SSH_FX_BAD_MESSAGE error. Ensure that SftpOutputStreamAsync does not produce such extra zero-byte writes.
1 parent bde5669 commit 32038f1

3 files changed

Lines changed: 133 additions & 17 deletions

File tree

‎CHANGES.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
* [GH-827](https://github.com/apache/mina-sshd/issues/827) Don't fail on invalid `known_hosts` lines; log and skip them
3535
* [GH-830](https://github.com/apache/mina-sshd/issues/830) EC public keys: let Bouncy Castle generate X.509 encodings with the curve OID as algorithm parameter
3636
* [GH-856](https://github.com/apache/mina-sshd/issues/856) Fix using ed25519 with BC-FIPS
37+
* [GH-861](https://github.com/apache/mina-sshd/issues/861) SFTP client: prevent sending zero-length writes in `SftpOutputStreamAsync`
3738

3839
* [SSHD-1348](https://issues.apache.org/jira/browse/SSHD-1348) Fix zero-length SFTP reads
3940

‎sshd-sftp/src/main/java/org/apache/sshd/sftp/client/impl/SftpOutputStreamAsync.java‎

Lines changed: 31 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,12 @@ private long internalTransfer(ByteInput stream, boolean forceFlush) throws IOExc
290290

291291
@Override
292292
public void flush() throws IOException {
293-
internalFlush();
293+
if (!isOpen()) {
294+
throw new IOException("flush(" + getPath() + ") stream is closed");
295+
}
296+
if (buffer != null && buffer.available() > 0) {
297+
internalFlush();
298+
}
294299
if (lastMsg != null) {
295300
lastMsg.waitUntilSent();
296301
lastMsg = null;
@@ -307,8 +312,8 @@ private void internalFlush() throws IOException {
307312
for (int ackIndex = 1;; ackIndex++) {
308313
SftpAckData ack = pendingAcks.peek();
309314
if (ack == null) {
310-
if (debugEnabled) {
311-
log.debug("flush({}) processed {} pending writes", this, ackIndex);
315+
if (debugEnabled && ackIndex > 1) {
316+
log.debug("flush({}) processed {} pending writes", this, ackIndex - 1);
312317
}
313318
break;
314319
}
@@ -333,39 +338,48 @@ private void internalFlush() throws IOException {
333338
checkStatus(client, buf);
334339
}
335340

336-
if (buffer == null) {
341+
Buffer currentData = buffer;
342+
buffer = null;
343+
if (currentData == null) {
337344
if (debugEnabled) {
338345
log.debug("flush({}) no pending buffer to flush", this);
339346
}
340347
return;
341348
}
342349

343-
int avail = buffer.available();
350+
int avail = currentData.available();
351+
352+
if (avail == 0) {
353+
if (debugEnabled) {
354+
log.debug("flush({}) no pending data in buffer to flush", this);
355+
}
356+
return;
357+
}
358+
359+
long currentOffset = offset;
360+
offset += avail;
344361

345-
int wpos = buffer.wpos();
362+
int wpos = currentData.wpos();
346363
// 4 = handle length
347364
// handle bytes
348365
// 8 = file offset
349366
// 4 = length of actual data
350-
buffer.rpos(buffer.rpos() - 16 - handleId.length);
351-
buffer.wpos(buffer.rpos());
352-
buffer.putBytes(handleId);
353-
buffer.putLong(offset);
354-
buffer.putUInt(avail);
355-
buffer.wpos(wpos);
367+
currentData.rpos(currentData.rpos() - 16 - handleId.length);
368+
currentData.wpos(currentData.rpos());
369+
currentData.putBytes(handleId);
370+
currentData.putLong(currentOffset);
371+
currentData.putUInt(avail);
372+
currentData.wpos(wpos);
356373

357374
if (lastMsg != null) {
358375
lastMsg.waitUntilSent();
359376
}
360-
lastMsg = client.write(SftpConstants.SSH_FXP_WRITE, buffer);
361-
SftpAckData ack = new SftpAckData(lastMsg.getId(), offset, avail);
377+
lastMsg = client.write(SftpConstants.SSH_FXP_WRITE, currentData);
378+
SftpAckData ack = new SftpAckData(lastMsg.getId(), currentOffset, avail);
362379
if (debugEnabled) {
363380
log.debug("flush({}) enqueue pending ack={}", this, ack);
364381
}
365382
pendingAcks.add(ack);
366-
367-
offset += avail;
368-
buffer = null;
369383
}
370384

371385
private void checkStatus(AbstractSftpClient client, Buffer buf) throws IOException {

‎sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpClientTest.java‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.apache.sshd.sftp.client;
20+
21+
import java.io.IOException;
22+
import java.io.InputStream;
23+
import java.io.OutputStream;
24+
import java.nio.file.Files;
25+
import java.nio.file.Path;
26+
import java.util.concurrent.ThreadLocalRandom;
27+
import java.util.concurrent.atomic.AtomicInteger;
28+
29+
import org.apache.sshd.common.util.io.IoUtils;
30+
import org.apache.sshd.server.session.ServerSession;
31+
import org.apache.sshd.server.subsystem.SubsystemFactory;
32+
import org.apache.sshd.sftp.SftpModuleProperties;
33+
import org.apache.sshd.sftp.common.SftpConstants;
34+
import org.apache.sshd.sftp.server.FileHandle;
35+
import org.apache.sshd.sftp.server.SftpEventListener;
36+
import org.apache.sshd.sftp.server.SftpSubsystemFactory;
37+
import org.apache.sshd.util.test.CommonTestSupportUtils;
38+
import org.junit.jupiter.api.Test;
39+
40+
class SftpClientTest extends AbstractSftpClientTestSupport {
41+
42+
SftpClientTest() {
43+
super();
44+
}
45+
46+
@Test
47+
void writeExactBufferSize() throws Exception {
48+
SftpSubsystemFactory sftpFactory = null;
49+
for (SubsystemFactory f : sshd.getSubsystemFactories()) {
50+
if (f instanceof SftpSubsystemFactory) {
51+
sftpFactory = (SftpSubsystemFactory) f;
52+
break;
53+
}
54+
}
55+
assertNotNull(sftpFactory);
56+
57+
AtomicInteger zeroWrite = new AtomicInteger();
58+
SftpEventListener listener = new SftpEventListener() {
59+
60+
@Override
61+
public void writing(
62+
ServerSession session, String remoteHandle, FileHandle localHandle, long offset, byte[] data,
63+
int dataOffset, int dataLen) throws IOException {
64+
if (dataLen == 0) {
65+
zeroWrite.incrementAndGet();
66+
}
67+
}
68+
};
69+
70+
sftpFactory.addSftpEventListener(listener);
71+
72+
Path targetPath = detectTargetFolder();
73+
Path parentPath = targetPath.getParent();
74+
Path lclSftp = CommonTestSupportUtils.resolve(targetPath, SftpConstants.SFTP_SUBSYSTEM_NAME, getClass().getSimpleName(),
75+
getCurrentTestName());
76+
Path inputFile = targetPath.resolve("input.bin");
77+
Path testFile = assertHierarchyTargetFolderExists(lclSftp).resolve("file.bin");
78+
int packetSize = 32 * 1024;
79+
int handleSize = SftpModuleProperties.FILE_HANDLE_SIZE.getRequired(sshd).intValue();
80+
int payloadSize = packetSize - 30 - handleSize;
81+
byte[] expected = new byte[payloadSize];
82+
83+
ThreadLocalRandom.current().nextBytes(expected);
84+
try (SftpClient sftp = createSingleSessionClient()) {
85+
Files.write(inputFile, expected);
86+
String file = CommonTestSupportUtils.resolveRelativeRemotePath(parentPath, testFile);
87+
try (InputStream in = Files.newInputStream(inputFile); OutputStream out = sftp.write(file)) {
88+
IoUtils.copy(in, out, 32 * 1024);
89+
out.flush(); // This flush caused a zero-bytes write; see GH-861
90+
// Note: the implicit flush on out.close() did not produce such a zero-bytes write.
91+
}
92+
byte[] transferred = Files.readAllBytes(testFile);
93+
assertArrayEquals(expected, transferred);
94+
} finally {
95+
Files.deleteIfExists(inputFile);
96+
sftpFactory.removeSftpEventListener(listener);
97+
}
98+
assertEquals(0, zeroWrite.get(), "Unexpected SSH_FXP_WRITE with zero data bytes");
99+
}
100+
101+
}

0 commit comments

Comments
 (0)