--- Begin Message ---
Package: john
Version: 1.6.36-4
Severity: normal
Tags: experimental patch
Hello,
I'm experiencing some segmentation faults with the --restore and
--status options. Here is how it can be reproduced:
$ /usr/sbin/john testcase.2
After a few seconds I stop the session (Ctrl-C)
the john.rec was created in my ~/.john directory
I'm using john with the following testcase (any non-trivial password
should be OK):
root:RfiUAvoW5xYKk:0:0:root:/root:/bin/bash
(testcase.2 with a one character diff)
Then I wanted to restore it:
$ /usr/sbin/john --restore
Segmentation fault
Asking for the status leads to another segfaut:
$ /usr/sbin/john --status
Segmentation fault
I'm giving lots of details of my bug analysis, because I'm not sure of
the attached patch.
The segmentation fault occurs in path_expand which is called with
a NULL filename.
Fixing path_expand with "if (!name) return NULL;" won't help, because
the problem is that there is no session name.
In the upstream source, there is a default session name (RECOVERY_NAME),
but the --private patch initialized rec_name to NULL, and set it later to
private_path(RECOVERY_NAME) in rec_init.
In the case of the --status and --restore options, rec_restore_args is
called before rec_init. So I added:
if ( rec_name == NULL )
rec_name = private_path(RECOVERY_NAME);
at the beginning of rec_restore_args (note: it is still needed for rec_init)
But then, it fails in path_expand again, because path_init wasn't called
before option_init, and user_home_path is empty.
The call order of opt_init and path_init was changed in Debian to add
the --private option (which made path_init dependant of options).
The patch I propose is to split path_init in two parts, the second part
will be in charge to create the private directory and will be called
after opt_init (and I hope the private directory is not needed for
status_init and opt_init).
With the attached patch, I noticed the following behavior:
"john --status" will use the ~/.john/john.rec file, even if the user is
in the directory of a previous session. He will then have to use either
"john --private=. --status" or "john --status=john"
I don't know if it is the expected behavior.
By the way, the patch fix some other issues:
- in private_path, size was too short by one byte => the string can be
non-'\0' terminated which leads to segfaults (this was needed for a
proper operation of the patch)
- add the OPT_REQ_PARAM flag to --config and --private (this should
fix the segmentation fault mentioned in the TODO file).
- remove the FLG_PRIVATE flag to allow using --private with other
options. I test options.private to know if the --private option was
set.
- remove some compilation warning:
+ change prototypes of dummy_rules_apply, and the apply types in
wordlist.c
+ replace malloc by mem_alloc in private_path
+ include <unistd.h> for _exit in signals.c
(Note: new upstream version also fix the strict aliasing warnings)
Note:
There is a very simple solution to avoid those segmentation faults with
the current package:
using a named session (with --session=foo and --restore=foo, for
example, --status=john in the ~/.john directory will show the status of
my previous session).
[Update]:
I wanted to know how the cronjob could restore its session, and have
seen that it was possible to provide the path of the session. I tried:
$ john --status=~/.john/john
fopen: ~/.john/john: No such file or directory
This unfortunately doesn't work because of the dot
$ john --status=~/.john/john.rec
will work. A second patch is attached which should fix this.
This one could probably be forwarded upstream.
I still think the first patch should be applied.
-- System Information:
Debian Release: 3.1
APT prefers unstable
APT policy: (500, 'unstable'), (1, 'experimental')
Architecture: i386 (i686)
Kernel: Linux 2.4.26
Locale: LANG=fr_FR.UTF-8, LC_CTYPE=fr_FR.UTF-8
Versions of packages john depends on:
ii debconf 1.4.30 Debian configuration management sy
ii libc6 2.3.2.ds1-15 GNU C Library: Shared libraries an
-- debconf information:
john/noconfigfile:
john/wordlist: /usr/share/john/password.lst
* john/cronjob: true
* john/cronjob-replacement: true
john/no-replacement:
Kind Regards,
--
Nekral
diff -raup ../orig/src/john.c src/john.c
--- ../orig/src/john.c 2004-08-12 22:51:54.000000000 +0200
+++ src/john.c 2004-08-13 15:48:14.000000000 +0200
@@ -231,9 +231,10 @@ static void john_init(int argc, char **a
#endif
+ path_init(argv);
status_init(NULL, 1);
opt_init(argc, argv);
- path_init(argv);
+ create_private_home();
if (options.flags & FLG_CFGFILE)
/* If the user has defined a config file, use this one or exit */
cfg_init(options.config,0);
diff -raup ../orig/src/options.c src/options.c
--- ../orig/src/options.c 2004-08-12 22:51:54.000000000 +0200
+++ src/options.c 2004-08-13 15:48:14.000000000 +0200
@@ -67,10 +67,10 @@ static struct opt_entry opt_list[] = {
"%u", &mem_saving_level},
/* Added to have personalised locations of files */
{"config", FLG_CFGFILE, FLG_CRACKING_CHK,
- 0,0, OPT_FMT_STR_ALLOC, &options.config},
+ 0, OPT_REQ_PARAM, OPT_FMT_STR_ALLOC, &options.config},
#ifdef JOHN_PRIVATE_HOME
- {"private", FLG_PRIVATE, FLG_CRACKING_CHK,
- 0,0, OPT_FMT_STR_ALLOC, &options.private},
+ {"private", 0, 0,
+ 0, OPT_REQ_PARAM, OPT_FMT_STR_ALLOC, &options.private},
#endif
{NULL}
};
@@ -130,6 +130,7 @@ void opt_init(int argc, char **argv)
list_init(&options.loader.users);
list_init(&options.loader.groups);
list_init(&options.loader.shells);
+ options.private = NULL;
options.length = -1;
diff -raup ../orig/src/options.h src/options.h
--- ../orig/src/options.h 2004-08-12 22:51:54.000000000 +0200
+++ src/options.h 2004-08-13 15:48:14.000000000 +0200
@@ -85,9 +85,6 @@
/* Configuration file set on command line */
#define FLG_CFGFILE 0x08000000
/* Private directory set on command line */
-#ifdef JOHN_PRIVATE_HOME
-#define FLG_PRIVATE 0x10000000
-#endif
/*
* Structure with option flags and all the parameters.
diff -raup ../orig/src/path.c src/path.c
--- ../orig/src/path.c 2004-08-12 22:51:54.000000000 +0200
+++ src/path.c 2004-08-13 15:48:14.000000000 +0200
@@ -29,9 +29,6 @@ void path_init(char **argv)
{
#if JOHN_SYSTEMWIDE
struct passwd *pw;
-#ifdef JOHN_PRIVATE_HOME
- char *private;
-#endif
#else
char *pos;
#endif
@@ -53,8 +50,24 @@ void path_init(char **argv)
memcpy(user_home_path, pw->pw_dir, user_home_length - 1);
user_home_path[user_home_length - 1] = '/';
+#else
+ if (argv[0])
+ if (!john_home_path && (pos = strrchr(argv[0], '/'))) {
+ john_home_length = pos - argv[0] + 1;
+ if (john_home_length >= PATH_BUFFER_SIZE) return;
+
+ john_home_path = mem_alloc(PATH_BUFFER_SIZE);
+ memcpy(john_home_path, argv[0], john_home_length);
+ }
+#endif
+}
+
+void create_private_home()
+{
#ifdef JOHN_PRIVATE_HOME
- if (options.flags & FLG_PRIVATE)
+ char *private;
+
+ if (options.private)
/* If the user has defined an alternate private file use it */
private = path_expand(options.private);
else
@@ -67,16 +80,6 @@ void path_init(char **argv)
fprintf(stderr, "Consider creating your own %s/john.ini
configuration file\n(in the meantime the global configuration file will be
used, if possible)\n", private);
}
#endif
-#else
- if (argv[0])
- if (!john_home_path && (pos = strrchr(argv[0], '/'))) {
- john_home_length = pos - argv[0] + 1;
- if (john_home_length >= PATH_BUFFER_SIZE) return;
-
- john_home_path = mem_alloc(PATH_BUFFER_SIZE);
- memcpy(john_home_path, argv[0], john_home_length);
- }
-#endif
}
char *path_expand(char *name)
@@ -126,16 +129,14 @@ char *private_path(char *path)
char *ploc;
char *ppath;
unsigned int size = 0;
- if ( ! options.flags & FLG_PRIVATE )
- return path;
if ( options.private == NULL || strlen(options.private) == 0 )
return path;
/* If the user has defined an alternate private file use it */
ploc = strstr(path,"john.");
if ( ploc == NULL )
return path;
- size = (strlen(options.private)+strlen(ploc)+1)*sizeof(char);
- ppath = malloc(size);
+ size = (strlen(options.private)+strlen(ploc)+2)*sizeof(char);
+ ppath = mem_alloc(size);
ppath = strncpy(ppath,options.private,size);
ppath = strcat(ppath,"/");
ppath = strcat(ppath,ploc);
diff -raup ../orig/src/path.h src/path.h
--- ../orig/src/path.h 2004-08-12 22:51:54.000000000 +0200
+++ src/path.h 2004-08-13 15:48:14.000000000 +0200
@@ -14,6 +14,7 @@
* Initializes the home directory path based on argv[0].
*/
extern void path_init(char **argv);
+extern void create_private_home();
/*
* Expands "$JOHN/" and "~/" in a path name.
diff -raup ../orig/src/recovery.c src/recovery.c
--- ../orig/src/recovery.c 2004-08-12 22:51:54.000000000 +0200
+++ src/recovery.c 2004-08-13 15:48:14.000000000 +0200
@@ -169,6 +169,8 @@ void rec_restore_args(int lock)
char **argv;
char *save_rec_name;
+ if ( rec_name == NULL )
+ rec_name = private_path(RECOVERY_NAME);
if (!(rec_file = fopen(path_expand(rec_name), "r+"))) {
save_rec_name = rec_name;
rec_name = rec_name_complete(rec_name);
diff -raup ../orig/src/signals.c src/signals.c
--- ../orig/src/signals.c 2003-09-06 12:30:28.000000000 +0200
+++ src/signals.c 2004-08-13 20:54:02.000000000 +0200
@@ -12,6 +12,7 @@
#include <limits.h>
#endif
#include <stdio.h>
+#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <signal.h>
diff -raup ../orig/src/wordlist.c src/wordlist.c
--- ../orig/src/wordlist.c 2003-09-06 22:55:41.000000000 +0200
+++ src/wordlist.c 2004-08-13 15:48:14.000000000 +0200
@@ -119,7 +119,7 @@ static int get_progress(void)
div64by32lo(&x100, file_stat.st_size + 1)) / rule_count;
}
-static char *dummy_rules_apply(char *word, char *rule, int split)
+static char *dummy_rules_apply(char *word, unsigned char *rule, int split)
{
word[length] = 0;
@@ -132,7 +132,7 @@ void do_wordlist_crack(struct db_main *d
struct rpp_context ctx;
char *prerule, *rule, *word;
char last[RULE_WORD_SIZE];
- char *(*apply)(char *word, char *rule, int split);
+ char *(*apply)(char *word, unsigned char *rule, int split);
log_event("Proceeding with wordlist mode");
--- ../orig/src/recovery.c 2004-08-12 22:51:54.000000000 +0200
+++ src/recovery.c 2004-08-14 00:08:50.000000000 +0200
@@ -39,10 +39,16 @@ static FILE *rec_file = NULL;
static struct db_main *rec_db;
static void (*rec_save_mode)(FILE *file);
+/*
+ * If needed, complete the recovery session filename with RECOVERY_SUFFIX
+ */
static char *rec_name_complete(char *rec_name)
{
char *result;
- if (strchr(rec_name, '.')) return rec_name;
+ if (strlen(rec_name) >= strlen(RECOVERY_SUFFIX))
+ if (strncmp(rec_name+strlen(rec_name)-strlen(RECOVERY_SUFFIX),
+ RECOVERY_SUFFIX, strlen(rec_name)) == 0)
+ return rec_name;
result = mem_alloc_tiny(strlen(rec_name) +
strlen(RECOVERY_SUFFIX) + 1, MEM_ALIGN_NONE);
--- End Message ---
signature.asc
