HARUYAMA Seigo
haruy****@queen*****
2005年 4月 15日 (金) 15:22:26 JST
春山です. Message-ID:<877jjhjvri.wl%haruy****@queen*****> にひきつづき, 静的なソースコード監査ツール RATS - Rough Auditing Tool for Security http://www.securesoftware.com/resources/tools.html を用いてanthyのコードの一部をチェックしました. 今回は anthy-6331 の src-diclib ディレクトリ中のコードを対象としました. 1. xstr.c: anthy_sputxchar(), anthy_sputxstr() これらの関数は引数のbufferの大きさを考慮しないので, 潜在的な危険があります. anthy_sputxchar(): バッファの大きさを引数(buf_len)として取るようにし マクロ MAX_BYTES_PER_XCHAR より短いバッファの大きさがおくられた場合には 0 を返すように変更しました. この変更は src-diclib/xstr.c だけでなく include/xstr.h, mkanthydic/mkdic.c にも影響しています. また, strcpy()の前にコピー元がMAX_BYTES_PER_XCHARを越えていないかの チェックも加えました. buf_len は size_t 型としました. anthy_snputxstr()では int ですが size_tのほうが好ましいのではないかと. anthy_sputxstr(): すべて anthy_snputxstr(): でおきかえて, この関数は消しました. 2. recode.c: check_anthy_dir() stat()->mkdir()->chmod()としていますが mkdir() のみで十分に見えます. また, mkdir()したあとに このディレクトリ以上のディレクトリが ユーザと管理者以外から書き込めないことをチェックするのがよい, とおもい 「Secure Programming Cookbook for C and C++」 by John Viega and Matt Messier の Section2-4 (私がもっているのは 和訳の「C/C++セキュアプログラミングクックブック VOLUME 1」) 関数 spc_is_safedir() を適用しようとしました. (spc_is_safedir()はパッチとは別に添付します) が, % ls -ld /home drwxrwsr-x 5 root staff 4096 2004-04-29 11:52 /home/ となっている Debian unstable では staffグループから書きこみ可能と判断されて spc_is_safedir()が0(安全ではない)を返してしまいましたので, コメントアウトしてあります. (staffグループのユーザはデフォルトではいません) 案としては, a. このまま b. spc_is_safedir()から「| S_IWGRP)」を消して とりあえず other からの書きこみ ができないことを確認するのにつかう c. 現状ではログにだしているだけなので spc_is_safedir() をそのまま使う といったところでしょうか. 3. recode.c: read_journal_record() stat()したあとにfopen()してますが, 競合状態になりえます. fopen()したあとにfstat()するように変更しました. 4. xstr.c: 変数をフォーマット文字列としている *printf()関数を 変更しました. 例: - sprintf(&buf[l], b); + sprintf(&buf[l], "%s", b); logger.c: do_log() においても フォーマット文字列に変数を用いていますが, これは簡単には除けないので放置してあります. 案としては, フォーマット文字列 str を検査する くらいです. 現状 変換指定子として %s 以外は利用していないようなので, 「% の後の文字が [%ds] でないものは %を別の文字に変える」 あたりがいいかもしれません. %n のみを除くのはよくないでしょう. %とnの間になにか(引数の指定子など)が入る可能性があるので. パッチと spc_is_safedir()の実装を示します. パッチをあてて手元でコンパイルできることを確認していますが, 十分テストされたものではありません. spc_is_safedir()は http://www.secureprogramming.com/downloads/spc-1.1.tar.gz の spc-1.1/chapter2/4-safedir.c に含まれているそのものです. spc_is_safedir()はスレッドセーフではありません. -----patch ここから----- --- orig/include/xstr.h +++ mod/include/xstr.h @@ -6,6 +6,8 @@ #ifndef _xstr_h_included_ #define _xstr_h_included_ +#include <stdlib.h> + /** 文字型 * EUCが入っている */ typedef int xchar; @@ -26,8 +28,9 @@ void anthy_putxstrln(xstr *); /* Cの文字列への書き出し */ -int anthy_sputxchar(char *, xchar , int encoding); -int anthy_sputxstr(char *, xstr *); +/* int anthy_sputxchar(char *, xchar , int encoding); */ +int anthy_sputxchar(char *, size_t, xchar , int encoding); +/* int anthy_sputxstr(char *, xstr *); */ int anthy_snputxstr(char *, int , xstr *, int encoding); /* xstrとstr共にmallocされる、freeで両方解放するかanthy_free_xstrで解放する */ --- orig/mkanthydic/mkdic.c +++ mod/mkanthydic/mkdic.c @@ -191,8 +191,9 @@ fprintf(page_out, "%c", 1); } for (i = m; i < c-> len; i++) { - char buf[8]; - len += anthy_sputxchar(buf, c->str[i], 0); + /* char buf[8]; */ + char buf[10]; + len += anthy_sputxchar(buf, 10, c->str[i], 0); fputs(buf, page_out); } return len; --- orig/src-diclib/record.c +++ mod/src-diclib/record.c @@ -294,7 +294,8 @@ } } else { if (fp) { - anthy_sputxstr(buf, &n->l->column.key); + /* anthy_sputxstr(buf, &n->l->column.key); */ + anthy_snputxstr(buf, 1024, &n->l->column.key, 0); fprintf(fp, "%s\n", buf); } cnt = 1; @@ -310,7 +311,8 @@ } } else { if(fp) { - anthy_sputxstr(buf, &n->r->column.key); + /* anthy_sputxstr(buf, &n->r->column.key); */ + anthy_snputxstr(buf, 1024, &n->r->column.key, 0); fprintf(fp, "%s\n", buf); } return cnt + 1; @@ -985,6 +987,8 @@ hd = anthy_conf_get_str("HOME"); dn = alloca(strlen(hd) + 10); sprintf(dn, "%s/.anthy", hd); + +#if 0 if (stat(dn, &st) || !S_ISDIR(st.st_mode)) { int r; /*fprintf(stderr, "Anthy: Failed to open anthy directory(%s).\n", dn);*/ @@ -999,6 +1003,29 @@ anthy_log(0, "But failed to change permission.\n"); } } +#else + { + int r; + r = mkdir(dn, S_IRWXU); + if (r == -1){ + if (errno != EEXIST){ + anthy_log(0, "Failed to create profile directory\n"); + return ; + } + } + /* + r = spc_is_safedir(dn); + if(r == 0){ + anthy_log(0, "profile directory is probably not safe\n"); + return; + }else if(r < 0){ + anthy_log(0, "Error occured during checking profile directory\n"); + return; + } + */ + } + +#endif } /* 再読み込みの必要があるかをチェックする @@ -1198,13 +1225,18 @@ if (rs->is_anon) { return ; } - if (stat(rs->journal_fn, &st) == -1) { - return ; - } + /* if (stat(rs->journal_fn, &st) == -1) { + * return ; + * } + */ fp = fopen(rs->journal_fn, "r"); if (fp == NULL) { return; } + if (fstat(fileno(fp), &st) == -1) { + fclose(fp); + return ; + } if (st.st_size < rs->last_update) { /* 最初から読み込む */ fseek(fp, 0, SEEK_SET); @@ -1249,9 +1281,9 @@ write_quote_xstr(FILE* fp, xstr* xs) { char* buf; - - buf = (char*) alloca(xs->len * 6 + 2); /* EUC またはUTF8を仮定 */ - anthy_sputxstr(buf, xs); + size_t buf_len = xs->len * 6 + 2; + buf = (char*) alloca(buf_len); /* EUC またはUTF8を仮定 */ + anthy_snputxstr(buf, buf_len, xs, 0); write_quote_string(fp, buf); } @@ -1437,14 +1469,16 @@ save_a_column(FILE *fp, struct record_column *c, int dirty) { int i; - char *buf = alloca(c->key.len * 6 + 2); + size_t buf_len = c->key.len * 6 + 2; + char *buf = alloca(buf_len); + /* LRUのマークを出力 */ if (dirty == 0) { fputc('-', fp); } else { fputc('+', fp); } - anthy_sputxstr(buf, &c->key); + anthy_snputxstr(buf, buf_len, &c->key, 0); /* index を出力 */ fprintf(fp, "%s ", buf); /**/ --- orig/src-diclib/xstr.c +++ mod/src-diclib/xstr.c @@ -334,9 +334,15 @@ free(x->str); } + +/* anthy_sputxchar(char *buf, xchar x, int encoding) */ int -anthy_sputxchar(char *buf, xchar x, int encoding) +anthy_sputxchar(char *buf, size_t buf_len, xchar x, int encoding) { + if(buf_len < MAX_BYTES_PER_XCHAR){ + return 0; + } + if (!xc_isprint(x)) { sprintf(buf, "??"); return 2; @@ -346,6 +352,10 @@ char tmp[10], *p; put_xchar_to_utf8_str(x, tmp); p = do_iconv(tmp, utf8_to_euc); + if (strlen(p) >= MAX_BYTES_PER_XCHAR){ + free(p); + return 0; + } strcpy(buf, p); free(p); return strlen(buf); @@ -366,30 +376,31 @@ #endif } +#if 0 int anthy_sputxstr(char *buf, xstr *x) { char b[MAX_BYTES_PER_XCHAR]; int i, l = 0; for (i = 0; i < x->len; i++) { - anthy_sputxchar(b, x->str[i], 0); - sprintf(&buf[l], b); + anthy_sputxchar(b, MAX_BYTES_PER_XCHAR, x->str[i], 0); + sprintf(&buf[l], "%s", b); l += strlen(b); } return l; } - +#endif int anthy_snputxstr(char *buf, int n, xstr *x, int encoding) { char b[MAX_BYTES_PER_XCHAR]; int i, l=0; for (i = 0; i < x->len; i++) { - anthy_sputxchar(b, x->str[i], encoding); + anthy_sputxchar(b, MAX_BYTES_PER_XCHAR, x->str[i], encoding); if ((int)strlen(b) + l >= n) { return l; } - n -= sprintf(&buf[l], b); + n -= sprintf(&buf[l], "%s", b); l += strlen(b); } return l; @@ -403,8 +414,8 @@ printf("\\%x", x); return ; } - anthy_sputxchar(buf, x, print_encoding); - printf(buf); + anthy_sputxchar(buf, MAX_BYTES_PER_XCHAR, x, print_encoding); + printf("%s", buf); } void -----ここまで----- -----spc_is_safedir()ここから----- #include <sys/types.h> #include <sys/stat.h> #include <dirent.h> #include <fcntl.h> #include <limits.h> #include <stdlib.h> #include <stdio.h> #include <unistd.h> int spc_is_safedir(const char *dir) { DIR *fd, *start; int rc = -1; char new_dir[PATH_MAX + 1]; uid_t uid; struct stat f, l; if (!(start = opendir("."))) return -1; if (lstat(dir, &l) == -1) { closedir(start); return -1; } uid = geteuid(); do { if (chdir(dir) == -1) break; if (!(fd = opendir("."))) break; if (fstat(dirfd(fd), &f) == -1) { closedir(fd); break; } closedir(fd); if (l.st_mode != f.st_mode || l.st_ino != f.st_ino || l.st_dev != f.st_dev) break; if ((f.st_mode & (S_IWOTH | S_IWGRP)) || (f.st_uid && f.st_uid != uid)) { rc = 0; break; } dir = ".."; if (lstat(dir, &l) == -1) break; if (!getcwd(new_dir, PATH_MAX + 1)) break; } while (new_dir[1]); /* new_dir[0] will always be a slash */ if (!new_dir[1]) rc = 1; fchdir(dirfd(start)); closedir(start); return rc; } -----ここまで----- -- 春山 征吾 / HARUYAMA Seigo haruy****@unixu***** haruy****@queen*****
