Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ENT-12033: Made multiple changes to increase robustness of remote file copying #5620

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

ENT-12033: Made multiple changes to increase robustness of remote file copying #5620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
23441a5
Added doc string to CopyRegularFileNet
larsewi Oct 21, 2024
6a383db
Added assert to check return code of snprintf
larsewi Oct 21, 2024
7a905fd
Fixed typo in comment for CopyRegularFileNet
larsewi Oct 21, 2024
4726972
Removed check for filename too long
larsewi Oct 21, 2024
226f64d
Removed unnecessary assignment of NULL-byte to buffer
larsewi Oct 21, 2024
d00d281
Check return code of sscanf
larsewi Oct 21, 2024
8daa59b
Added sanity check in CopyRegularFileNet
larsewi Oct 21, 2024
dadf891
Allocate receive buffer on stack instead of heap
larsewi Oct 21, 2024
a026630
cf-serverd now stats file every read on network transmission
larsewi Oct 21, 2024
68ffb07
Added filestream flushing on error in EncryptCopyRegularFileNet
larsewi Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 5 additions & 15 deletions cf-serverd/server_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ static void FailedTransfer(ConnectionInfo *connection)
void CfGetFile(ServerFileGetState *args)
{
int fd;
off_t n_read, total = 0, sendlen = 0, count = 0;
off_t n_read, total = 0, sendlen = 0;
char sendbuffer[CF_BUFSIZE + 256], filename[CF_BUFSIZE - 128];
struct stat sb;
int blocksize = 2048;
Expand Down Expand Up @@ -459,13 +459,6 @@ void CfGetFile(ServerFileGetState *args)
}
else
{
int div = 3;

if (sb.st_size > 10485760L) /* File larger than 10 MB, checks every 64kB */
{
div = 32;
}
larsewi marked this conversation as resolved.
Show resolved Hide resolved

while (true)
{
memset(sendbuffer, 0, CF_BUFSIZE);
Expand All @@ -488,14 +481,11 @@ void CfGetFile(ServerFileGetState *args)

/* check the file is not changing at source */

if (count++ % div == 0) /* Don't do this too often */
if (stat(filename, &sb) == -1)
{
if (stat(filename, &sb))
{
Log(LOG_LEVEL_ERR, "Cannot stat file '%s'. (stat: %s)",
filename, GetErrorStr());
break;
}
Log(LOG_LEVEL_ERR, "Cannot stat file '%s'. (stat: %s)",
filename, GetErrorStr());
break;
}

if (sb.st_size != savedlen)
Expand Down
130 changes: 63 additions & 67 deletions libcfnet/client_code.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,23 +568,32 @@ bool CompareHashNet(const char *file1, const char *file2, bool encrypt, AgentCon

/*********************************************************************/

static void FlushFileStream(int sd, int toget)
{
int i;
char buffer[2];

Log(LOG_LEVEL_VERBOSE, "Flushing rest of file...%d bytes", toget);

for (i = 0; i < toget; i++)
{
recv(sd, buffer, 1, 0); /* flush to end of current file */
}
}

static bool EncryptCopyRegularFileNet(const char *source, const char *dest, off_t size, AgentConnection *conn)
{
assert(conn != NULL);

int blocksize = 2048, n_read = 0, plainlen, more = true, finlen;
int tosend, cipherlen = 0;
char *buf, in[CF_BUFSIZE], out[CF_BUFSIZE], workbuf[CF_BUFSIZE], cfchangedstr[265];
// TODO: CFE-4449
char buf[CF_BUFSIZE + sizeof(int)], in[CF_BUFSIZE], out[CF_BUFSIZE], workbuf[CF_BUFSIZE], cfchangedstr[265];
unsigned char iv[32] =
{ 1, 2, 3, 4, 5, 6, 7, 8, 1, 2, 3, 4, 5, 6, 7, 8, 1, 2, 3, 4, 5, 6, 7, 8, 1, 2, 3, 4, 5, 6, 7, 8 };

snprintf(cfchangedstr, 255, "%s%s", CF_CHANGEDSTR1, CF_CHANGEDSTR2);

if ((strlen(dest) > CF_BUFSIZE - 20))
{
Log(LOG_LEVEL_ERR, "Filename too long");
return false;
}

unlink(dest); /* To avoid link attacks */

int dd = safe_open_create_perms(dest, O_WRONLY | O_CREAT | O_TRUNC | O_EXCL | O_BINARY, CF_PERMS_DEFAULT);
Expand All @@ -604,9 +613,8 @@ static bool EncryptCopyRegularFileNet(const char *source, const char *dest, off_
return true;
}

workbuf[0] = '\0';

snprintf(in, CF_BUFSIZE - CF_PROTO_OFFSET, "GET dummykey %s", source);
NDEBUG_UNUSED int rc = snprintf(in, CF_BUFSIZE - CF_PROTO_OFFSET, "GET dummykey %s", source);
assert(rc >= 0 && rc < sizeof(in));
cipherlen = EncryptString(out, sizeof(out), in, strlen(in) + 1, conn->encryption_type, conn->session_key);

tosend = cipherlen + CF_PROTO_OFFSET;
Expand All @@ -621,7 +629,8 @@ static bool EncryptCopyRegularFileNet(const char *source, const char *dest, off_
tosend, sizeof(workbuf));
}

snprintf(workbuf, CF_BUFSIZE, "SGET %4d %4d", cipherlen, blocksize);
rc = snprintf(workbuf, CF_BUFSIZE, "SGET %4d %4d", cipherlen, blocksize);
assert(rc >= 0 && rc < sizeof(workbuf));
memcpy(workbuf + CF_PROTO_OFFSET, out, cipherlen);

/* Send proposition C0 - query */
Expand All @@ -642,8 +651,6 @@ static bool EncryptCopyRegularFileNet(const char *source, const char *dest, off_
return false;
}

buf = xmalloc(CF_BUFSIZE + sizeof(int));

bool last_write_made_hole = false;
size_t n_wrote_total = 0;

Expand All @@ -652,7 +659,6 @@ static bool EncryptCopyRegularFileNet(const char *source, const char *dest, off_
if ((cipherlen = ReceiveTransaction(conn->conn_info, buf, &more)) == -1)
{
close(dd);
free(buf);
EVP_CIPHER_CTX_free(crypto_ctx);
return false;
}
Expand All @@ -665,7 +671,6 @@ static bool EncryptCopyRegularFileNet(const char *source, const char *dest, off_
{
Log(LOG_LEVEL_INFO, "Network access to '%s:%s' denied", conn->this_server, source);
close(dd);
free(buf);
EVP_CIPHER_CTX_free(crypto_ctx);
return false;
}
Expand All @@ -674,7 +679,6 @@ static bool EncryptCopyRegularFileNet(const char *source, const char *dest, off_
{
Log(LOG_LEVEL_INFO, "Source '%s:%s' changed while copying", conn->this_server, source);
close(dd);
free(buf);
EVP_CIPHER_CTX_free(crypto_ctx);
return false;
}
Expand All @@ -683,16 +687,16 @@ static bool EncryptCopyRegularFileNet(const char *source, const char *dest, off_

if (!EVP_DecryptUpdate(crypto_ctx, (unsigned char *) workbuf, &plainlen, (unsigned char *) buf, cipherlen))
{
FlushFileStream(conn->conn_info->sd, size - n_wrote_total - n_read);
close(dd);
free(buf);
EVP_CIPHER_CTX_free(crypto_ctx);
return false;
}

if (!EVP_DecryptFinal_ex(crypto_ctx, (unsigned char *) workbuf + plainlen, &finlen))
{
FlushFileStream(conn->conn_info->sd, size - n_wrote_total - n_read);
close(dd);
free(buf);
EVP_CIPHER_CTX_free(crypto_ctx);
return false;
}
Expand All @@ -701,20 +705,27 @@ static bool EncryptCopyRegularFileNet(const char *source, const char *dest, off_

bool w_ok = FileSparseWrite(dd, workbuf, n_read,
&last_write_made_hole);
if (!w_ok)
n_wrote_total += n_read;
if (!w_ok || n_wrote_total > size)
{
Log(LOG_LEVEL_ERR,
"Local disk write failed copying '%s:%s' to '%s'",
conn->this_server, source, dest);
free(buf);
if (!w_ok)
{
Log(LOG_LEVEL_ERR,
"Local disk write failed copying '%s:%s' to '%s'",
conn->this_server, source, dest);
}
else // if (n_wrote_total > size)
{
Log(LOG_LEVEL_INFO, "Source '%s:%s' changed while copying",
conn->this_server, source);
}
FlushFileStream(conn->conn_info->sd, size - n_wrote_total);
unlink(dest);
close(dd);
conn->error = true;
EVP_CIPHER_CTX_free(crypto_ctx);
return false;
}

n_wrote_total += n_read;
}

const bool do_sync = false;
Expand All @@ -724,35 +735,24 @@ static bool EncryptCopyRegularFileNet(const char *source, const char *dest, off_
if (!ret)
{
unlink(dest);
free(buf);
EVP_CIPHER_CTX_free(crypto_ctx);
return false;
}

free(buf);
EVP_CIPHER_CTX_free(crypto_ctx);
return true;
}

static void FlushFileStream(int sd, int toget)
{
int i;
char buffer[2];

Log(LOG_LEVEL_VERBOSE, "Flushing rest of file...%d bytes", toget);

for (i = 0; i < toget; i++)
{
recv(sd, buffer, 1, 0); /* flush to end of current file */
}
}

/* TODO finalise socket or TLS session in all cases that this function fails
/* TODO finalize socket or TLS session in all cases that this function fails
* and the transaction protocol is out of sync. */
bool CopyRegularFileNet(const char *source, const char *dest, off_t size,
bool encrypt, AgentConnection *conn, mode_t mode)
{
char *buf, workbuf[CF_BUFSIZE], cfchangedstr[265];
assert(conn != NULL);

// TODO: CFE-4449
char buf[CF_BUFSIZE + sizeof(int)]; /* Note CF_BUFSIZE not buf_size !! */
larsewi marked this conversation as resolved.
Show resolved Hide resolved
char workbuf[CF_BUFSIZE], cfchangedstr[265];
const int buf_size = 2048;

/* We encrypt only for CLASSIC protocol. The TLS protocol is always over
Expand All @@ -764,13 +764,8 @@ bool CopyRegularFileNet(const char *source, const char *dest, off_t size,
return EncryptCopyRegularFileNet(source, dest, size, conn);
}

snprintf(cfchangedstr, 255, "%s%s", CF_CHANGEDSTR1, CF_CHANGEDSTR2);

if ((strlen(dest) > CF_BUFSIZE - 20))
{
Log(LOG_LEVEL_ERR, "Filename too long");
return false;
}
larsewi marked this conversation as resolved.
Show resolved Hide resolved
NDEBUG_UNUSED int rc = snprintf(cfchangedstr, 255, "%s%s", CF_CHANGEDSTR1, CF_CHANGEDSTR2);
assert(rc >= 0 && rc < sizeof(cfchangedstr));

unlink(dest); /* To avoid link attacks */

Expand All @@ -784,7 +779,6 @@ bool CopyRegularFileNet(const char *source, const char *dest, off_t size,
return false;
}

workbuf[0] = '\0';
int tosend = snprintf(workbuf, CF_BUFSIZE, "GET %d %s", buf_size, source);
if (tosend <= 0 || tosend >= CF_BUFSIZE)
{
Expand All @@ -803,8 +797,6 @@ bool CopyRegularFileNet(const char *source, const char *dest, off_t size,
return false;
}

buf = xmalloc(CF_BUFSIZE + sizeof(int)); /* Note CF_BUFSIZE not buf_size !! */

Log(LOG_LEVEL_VERBOSE, "Copying remote file '%s:%s', expecting %jd bytes",
conn->this_server, source, (intmax_t)size);

Expand Down Expand Up @@ -848,7 +840,6 @@ bool CopyRegularFileNet(const char *source, const char *dest, off_t size,
conn->this_server, source, n_read);

close(dd);
free(buf);
return false;
}

Expand All @@ -860,7 +851,6 @@ bool CopyRegularFileNet(const char *source, const char *dest, off_t size,
Log(LOG_LEVEL_INFO, "Network access to '%s:%s' denied",
conn->this_server, source);
close(dd);
free(buf);
return false;
}

Expand All @@ -869,40 +859,48 @@ bool CopyRegularFileNet(const char *source, const char *dest, off_t size,
Log(LOG_LEVEL_INFO, "Source '%s:%s' changed while copying",
conn->this_server, source);
close(dd);
free(buf);
return false;
}

/* Check for mismatch between encryption here and on server. */

int value = -1;
sscanf(buf, "t %d", &value);
rc = sscanf(buf, "t %d", &value);
if (rc != 1)
{
value = -1;
}

if ((value > 0) && (strncmp(buf + CF_INBAND_OFFSET, "BAD: ", 5) == 0))
{
Log(LOG_LEVEL_INFO, "Network access to cleartext '%s:%s' denied",
conn->this_server, source);
close(dd);
free(buf);
return false;
}

bool w_ok = FileSparseWrite(dd, buf, n_read,
&last_write_made_hole);
if (!w_ok)
n_wrote_total += n_read;
if (!w_ok || n_wrote_total > size)
{
Log(LOG_LEVEL_ERR,
"Local disk write failed copying '%s:%s' to '%s'",
conn->this_server, source, dest);
free(buf);
if (!w_ok)
{
Log(LOG_LEVEL_ERR,
"Local disk write failed copying '%s:%s' to '%s'",
conn->this_server, source, dest);
}
else // if (n_wrote_total > size)
{
Log(LOG_LEVEL_INFO, "Source '%s:%s' changed while copying",
conn->this_server, source);
}
unlink(dest);
close(dd);
FlushFileStream(conn->conn_info->sd, size - n_wrote_total - n_read);
FlushFileStream(conn->conn_info->sd, size - n_wrote_total);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this depend on which error case we are handling? I'd probably just check n_wrote_total + n_read in the condition instead of doing the addition first.

conn->error = true;
return false;
}

n_wrote_total += n_read;
}

const bool do_sync = false;
Expand All @@ -912,11 +910,9 @@ bool CopyRegularFileNet(const char *source, const char *dest, off_t size,
if (!ret)
{
unlink(dest);
free(buf);
FlushFileStream(conn->conn_info->sd, size - n_wrote_total);
return false;
}

free(buf);
return true;
}
16 changes: 16 additions & 0 deletions libcfnet/client_code.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,24 @@ AgentConnection *ServerConnection(const char *server, const char *port, const Rl
void DisconnectServer(AgentConnection *conn);

bool CompareHashNet(const char *file1, const char *file2, bool encrypt, AgentConnection *conn);

/**
* @brief Copy regular file over network.
* @param source The source filename.
* @param dest The destination filename.
* @param size The source file size.
* @param encrypt Whether or not to encrypt the transmission.
* @param conn The connection object to communicate with server.
* @param mode The source file mode (perms) to be used when creating the
* destination file.
* @return %true on success, %false on error.
* @note Note that the #encrypt parameter only has an effect on the CLASSIC
* protocol (used in CFEngine Community). The TLS protocol (used in
* CFEngine Enterprise) is always encrypted.
*/
bool CopyRegularFileNet(const char *source, const char *dest, off_t size,
bool encrypt, AgentConnection *conn, mode_t mode);

Item *RemoteDirList(const char *dirname, bool encrypt, AgentConnection *conn);

int TLSConnectCallCollect(ConnectionInfo *conn_info, const char *username);
Expand Down
Loading