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.
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
Stack buffer overflow in tcmu_cdb_print_info #613
Comments
mikechristie
pushed a commit
to mikechristie/tcmu-runner
that referenced
this issue
Mar 13, 2020
Reported by Chris Coulson (chrisccoulson) at: open-iscsi#613 tcmu_cdb_print_info has a fixed size 193 bytes scratch buffer on the stack, defined here: void tcmu_cdb_print_info(struct tcmu_device *dev, const struct tcmulib_cmd *cmd, const char *info) { int i, n, bytes; char fix[CDB_FIX_SIZE], *buf; buf = fix; Further down, it uses sprintf to write some bytes in 2-digit hex format to buf, with each byte occupying 3 bytes in buf: for (i = 0, n = 0; i < bytes; i++) { n += sprintf(buf + n, "%x ", cmd->cdb[i]); } To avoid overflowing the stack buffer, there is a check earlier in the function which performs a heap allocation if bytes is too large: if (bytes > CDB_FIX_SIZE) { buf = malloc(CDB_TO_BUF_SIZE(bytes)); if (!buf) { tcmu_dev_err(dev, "out of memory\n"); return; } } But this check looks wrong. If bytes > 64, then the previously mentioned for loop will overflow the 193 byte stack buffer. I think it should be: if (bytes > CDB_FIX_BYTES) { Correcting this isn't sufficient on its own though, as this check doesn't take in to account that an additional string (info) is written to buf and can also result in the stack buffer overflowing: if (info) n += sprintf(buf + n, "%s", info); Signed-off-by: Mike Christie <mchristi@redhat.com>
Thanks! Agree on both cases. I made a PR that should hopefully address all your issues here: |
mikechristie
pushed a commit
to mikechristie/tcmu-runner
that referenced
this issue
Mar 13, 2020
Reported by Chris Coulson (chrisccoulson) at: open-iscsi#613 tcmu_cdb_print_info has a fixed size 193 bytes scratch buffer on the stack, defined here: void tcmu_cdb_print_info(struct tcmu_device *dev, const struct tcmulib_cmd *cmd, const char *info) { int i, n, bytes; char fix[CDB_FIX_SIZE], *buf; buf = fix; Further down, it uses sprintf to write some bytes in 2-digit hex format to buf, with each byte occupying 3 bytes in buf: for (i = 0, n = 0; i < bytes; i++) { n += sprintf(buf + n, "%x ", cmd->cdb[i]); } To avoid overflowing the stack buffer, there is a check earlier in the function which performs a heap allocation if bytes is too large: if (bytes > CDB_FIX_SIZE) { buf = malloc(CDB_TO_BUF_SIZE(bytes)); if (!buf) { tcmu_dev_err(dev, "out of memory\n"); return; } } But this check looks wrong. If bytes > 64, then the previously mentioned for loop will overflow the 193 byte stack buffer. I think it should be: if (bytes > CDB_FIX_BYTES) { Correcting this isn't sufficient on its own though, as this check doesn't take in to account that an additional string (info) is written to buf and can also result in the stack buffer overflowing: if (info) n += sprintf(buf + n, "%s", info); Signed-off-by: Mike Christie <mchristi@redhat.com>
runsisi
pushed a commit
to runsisi/tcmu-runner
that referenced
this issue
Jul 8, 2020
Reported by Chris Coulson (chrisccoulson) at: open-iscsi#613 tcmu_cdb_print_info has a fixed size 193 bytes scratch buffer on the stack, defined here: void tcmu_cdb_print_info(struct tcmu_device *dev, const struct tcmulib_cmd *cmd, const char *info) { int i, n, bytes; char fix[CDB_FIX_SIZE], *buf; buf = fix; Further down, it uses sprintf to write some bytes in 2-digit hex format to buf, with each byte occupying 3 bytes in buf: for (i = 0, n = 0; i < bytes; i++) { n += sprintf(buf + n, "%x ", cmd->cdb[i]); } To avoid overflowing the stack buffer, there is a check earlier in the function which performs a heap allocation if bytes is too large: if (bytes > CDB_FIX_SIZE) { buf = malloc(CDB_TO_BUF_SIZE(bytes)); if (!buf) { tcmu_dev_err(dev, "out of memory\n"); return; } } But this check looks wrong. If bytes > 64, then the previously mentioned for loop will overflow the 193 byte stack buffer. I think it should be: if (bytes > CDB_FIX_BYTES) { Correcting this isn't sufficient on its own though, as this check doesn't take in to account that an additional string (info) is written to buf and can also result in the stack buffer overflowing: if (info) n += sprintf(buf + n, "%s", info); Signed-off-by: Mike Christie <mchristi@redhat.com>
runsisi
pushed a commit
to runsisi/tcmu-runner
that referenced
this issue
Jul 8, 2020
Reported by Chris Coulson (chrisccoulson) at: open-iscsi#613 tcmu_cdb_print_info has a fixed size 193 bytes scratch buffer on the stack, defined here: void tcmu_cdb_print_info(struct tcmu_device *dev, const struct tcmulib_cmd *cmd, const char *info) { int i, n, bytes; char fix[CDB_FIX_SIZE], *buf; buf = fix; Further down, it uses sprintf to write some bytes in 2-digit hex format to buf, with each byte occupying 3 bytes in buf: for (i = 0, n = 0; i < bytes; i++) { n += sprintf(buf + n, "%x ", cmd->cdb[i]); } To avoid overflowing the stack buffer, there is a check earlier in the function which performs a heap allocation if bytes is too large: if (bytes > CDB_FIX_SIZE) { buf = malloc(CDB_TO_BUF_SIZE(bytes)); if (!buf) { tcmu_dev_err(dev, "out of memory\n"); return; } } But this check looks wrong. If bytes > 64, then the previously mentioned for loop will overflow the 193 byte stack buffer. I think it should be: if (bytes > CDB_FIX_BYTES) { Correcting this isn't sufficient on its own though, as this check doesn't take in to account that an additional string (info) is written to buf and can also result in the stack buffer overflowing: if (info) n += sprintf(buf + n, "%s", info); Signed-off-by: Mike Christie <mchristi@redhat.com>
lxbsz
pushed a commit
that referenced
this issue
Jul 31, 2020
Reported by Chris Coulson (chrisccoulson) at: #613 tcmu_cdb_print_info has a fixed size 193 bytes scratch buffer on the stack, defined here: void tcmu_cdb_print_info(struct tcmu_device *dev, const struct tcmulib_cmd *cmd, const char *info) { int i, n, bytes; char fix[CDB_FIX_SIZE], *buf; buf = fix; Further down, it uses sprintf to write some bytes in 2-digit hex format to buf, with each byte occupying 3 bytes in buf: for (i = 0, n = 0; i < bytes; i++) { n += sprintf(buf + n, "%x ", cmd->cdb[i]); } To avoid overflowing the stack buffer, there is a check earlier in the function which performs a heap allocation if bytes is too large: if (bytes > CDB_FIX_SIZE) { buf = malloc(CDB_TO_BUF_SIZE(bytes)); if (!buf) { tcmu_dev_err(dev, "out of memory\n"); return; } } But this check looks wrong. If bytes > 64, then the previously mentioned for loop will overflow the 193 byte stack buffer. I think it should be: if (bytes > CDB_FIX_BYTES) { Correcting this isn't sufficient on its own though, as this check doesn't take in to account that an additional string (info) is written to buf and can also result in the stack buffer overflowing: if (info) n += sprintf(buf + n, "%s", info); Signed-off-by: Mike Christie <mchristi@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
tcmu_cdb_print_info has a fixed size 193 bytes scratch buffer on the stack, defined here:
Further down, it uses sprintf to write some bytes in 2-digit hex format to buf, with each byte occupying 3 bytes in buf:
To avoid overflowing the stack buffer, there is a check earlier in the function which performs a heap allocation if bytes is too large:
But this check looks wrong. If bytes > 64, then the previously mentioned for loop will overflow the 193 byte stack buffer. I think it should be:
Correcting this isn't sufficient on its own though, as this check doesn't take in to account that an additional string (info) is written to buf and can also result in the stack buffer overflowing:
I would normally report this kind of issue privately, but it looks like triggering this code relies on changing the log level and AFAICT there isn't really a mechanism for an attacker to exploit this anyway.
The text was updated successfully, but these errors were encountered: