Create Task
Maniphest T80

torture_config_new test fails due to invalid session time out value caused by int pointer to long pointer cast
Closed, ResolvedPublic
Actions

Description

Hello,

the test torture_config_new, introduced in 53d84abb1 fails in torture_config.c on line 201 when running assert_int_equal(session->opts.timeout, 30).

The bug seems to appear due to the variable i being declared as an int on line 359 of config.c. The timeout value of 30 that is read previously is stored into the variable i.
Later on when the timeout is being set for the session, in ssh_options_set in options.c, the variable i is passed as const void *value.
On line 636 of options.c, the const void *value is cast to long * and assigned to long *x. Since on some architectures a long has eight bytes and an int only four, an additional four bytes of garbage are used when the pointer is dereferenced, so the test *x > 0 on line 637 fails which makes the torture_new_test fail too.

Line 650 has the same const void* to long * cast, so the same problem could happen for the option SSH_OPTIONS_TIMEOUT_USEC too.

session->opts.timeout is declared on line 196 of session.h as a unsigned long.
The timeout value read from the config file is assigned to i on line 510 of config.c, with i = ssh_config_get_int(&s -1).
Since different types are used at different places for the timeout and i is used at multiple places when parsing the config, im not sure what the best fix would be so I'm only reporting the bug for now.

Related Objects

jvijtiuk created this task.Dec 22 2017, 5:01 PM
Jakuje added a subscriber: Jakuje.Dec 24 2017, 12:16 AM

Thank you for the bug report with verbose analysis of the problem. I see you are referencing that the fail was introduced by my commit, but it was just adding a test that uncovered this problem that existed for longer time (from 484564261?). Fortunately, this is not very common configuration option from my experience so there was probably not a lot harm caused so far.

There is a function ssh_config_get_int(), which is internally using strtol() and the function itself is the main problem in the chain. It is only used in this single place so I believe renaming it to ssh_config_get_long() with respective types would resolve this problem (it is not in the public API).

The conversion from long to unsigned long in options.c should not a problem unless we would get negative timeouts and this is already excluded in config.c on line 512.

The attached patch should resolve the problem. Can you verify that on your architecture? I have only x86_64 at hand ...

From 6028ec0a559d040c8143a83510db3fca28cafaa1 Mon Sep 17 00:00:00 2001
From: Jakub Jelen <jjelen@redhat.com>
Date: Sat, 23 Dec 2017 23:31:20 +0100
Subject: [PATCH] config: Avoid long -> int -> long casting for timeout
 configuration option

Resolves:
https://bugs.libssh.org/T80
Signed-off-by: Jakub Jelen <jjelen@redhat.com>
---
 src/config.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/config.c b/src/config.c
index a95e0b63..99a1b332 100644
--- a/src/config.c
+++ b/src/config.c
@@ -251,9 +251,9 @@ out:
   return r;
 }
 
-static int ssh_config_get_int(char **str, int notfound) {
+static long ssh_config_get_long(char **str, long notfound) {
   char *p, *endp;
-  int i;
+  long i;
 
   p = ssh_config_get_token(str);
   if (p && *p) {
@@ -357,6 +357,7 @@ static int ssh_config_parse_line(ssh_session session, const char *line,
   char *lowerhost;
   size_t len;
   int i;
+  long l;
 
   x = s = strdup(line);
   if (s == NULL) {
@@ -507,9 +508,9 @@ static int ssh_config_parse_line(ssh_session session, const char *line,
       }
       break;
     case SOC_TIMEOUT:
-      i = ssh_config_get_int(&s, -1);
-      if (i >= 0 && *parsing) {
-        ssh_options_set(session, SSH_OPTIONS_TIMEOUT, &i);
+      l = ssh_config_get_long(&s, -1);
+      if (l >= 0 && *parsing) {
+        ssh_options_set(session, SSH_OPTIONS_TIMEOUT, &l);
       }
       break;
     case SOC_STRICTHOSTKEYCHECK:
-- 
2.14.3
jvijtiuk added a comment.Dec 27 2017, 4:07 PM

I see you are referencing that the fail was introduced by my commit, but it was just adding a test that uncovered this problem that existed for longer time (from 484564261?).

Yes, you are right, I should have worded that better, the torture_config_new test only made the bug visible.

Can you verify that on your architecture? I have only x86_64 at hand ...

The patch works for me but I have also only tested on x86_64.