From b566ad5a1713525b802669dd3b096580d9e3ae13 Mon Sep 17 00:00:00 2001 From: b1v1r Date: Thu, 5 Mar 2009 17:57:50 +0000 Subject: [PATCH] Prepare 2.5.8 release fixing MODSEC-27. --- .cdtproject | 1192 ------ .jupiter | 200 - .project | 85 - .settings/org.eclipse.cdt.core.prefs | 3 - CHANGES | 5 +- apache2/msc_release.h | 4 +- apache2/pdf_protect.c | 7 +- apache2/t/action/.empty | 0 apache2/t/regression/misc/20-pdf-xss.t | 54 + apache2/t/regression/server_root/data/.empty | 0 .../t/regression/server_root/htdocs/test.pdf | Bin 0 -> 15406 bytes .../regression/server_root/logs/audit/.empty | 0 .../regression/server_root/logs/subdir/.empty | 0 apache2/t/regression/server_root/tmp/.empty | 0 .../t/regression/server_root/upload/.empty | 0 doc/modsecurity2-apache-reference.xml | 2 +- review/pre-2.5-brian.review | 3436 ----------------- review/pre-2.5-brian.txt | 982 ----- review/review-summary.pl | 50 - 19 files changed, 65 insertions(+), 5955 deletions(-) delete mode 100644 .cdtproject delete mode 100644 .jupiter delete mode 100644 .project delete mode 100644 .settings/org.eclipse.cdt.core.prefs create mode 100644 apache2/t/action/.empty create mode 100644 apache2/t/regression/misc/20-pdf-xss.t create mode 100644 apache2/t/regression/server_root/data/.empty create mode 100644 apache2/t/regression/server_root/htdocs/test.pdf create mode 100644 apache2/t/regression/server_root/logs/audit/.empty create mode 100644 apache2/t/regression/server_root/logs/subdir/.empty create mode 100644 apache2/t/regression/server_root/tmp/.empty create mode 100644 apache2/t/regression/server_root/upload/.empty delete mode 100644 review/pre-2.5-brian.review delete mode 100644 review/pre-2.5-brian.txt delete mode 100755 review/review-summary.pl diff --git a/.cdtproject b/.cdtproject deleted file mode 100644 index 93d4fd4b..00000000 --- a/.cdtproject +++ /dev/null @@ -1,1192 +0,0 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - make - - all - false - true - - - make - - test - false - true - - - gksudo - make - install - false - false - - - make - - clean - false - true - - - make - - dist-clean - false - true - - - - - - - - - - - - - - - - - - - - - - - - - \ No newline at end of file diff --git a/.jupiter b/.jupiter deleted file mode 100644 index 4bd771a5..00000000 --- a/.jupiter +++ /dev/null @@ -1,200 +0,0 @@ - - - - property.default.description - - 1970-01-01 :: 00:00:00:000 GMT-10:00 - review - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - Pre 2.5 Review - brian - 2008-01-04 :: 10:09:23:000 GMT-08:00 - review - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - diff --git a/.project b/.project deleted file mode 100644 index 6f0bc9dc..00000000 --- a/.project +++ /dev/null @@ -1,85 +0,0 @@ - - - ModSecurity - - - - - - org.eclipse.cdt.make.core.makeBuilder - clean,full,incremental, - - - org.eclipse.cdt.make.core.append_environment - true - - - org.eclipse.cdt.make.core.enableCleanBuild - true - - - org.eclipse.cdt.make.core.build.command - make - - - org.eclipse.cdt.make.core.useDefaultBuildCmd - true - - - org.eclipse.cdt.make.core.build.target.auto - all - - - org.eclipse.cdt.make.core.stopOnError - false - - - org.eclipse.cdt.make.core.build.location - - - - org.eclipse.cdt.make.core.build.target.inc - all - - - org.eclipse.cdt.make.core.build.arguments - - - - org.eclipse.cdt.core.errorOutputParser - org.eclipse.cdt.core.MakeErrorParser;org.eclipse.cdt.core.GCCErrorParser;org.eclipse.cdt.core.GASErrorParser;org.eclipse.cdt.core.GLDErrorParser;org.eclipse.cdt.core.VCErrorParser; - - - org.eclipse.cdt.make.core.enableAutoBuild - false - - - org.eclipse.cdt.make.core.environment - - - - org.eclipse.cdt.make.core.enabledIncrementalBuild - true - - - org.eclipse.cdt.make.core.build.target.clean - clean - - - org.eclipse.cdt.make.core.enableFullBuild - true - - - - - org.eclipse.cdt.make.core.ScannerConfigBuilder - - - - - - org.eclipse.cdt.core.cnature - org.eclipse.cdt.make.core.makeNature - org.eclipse.cdt.make.core.ScannerConfigNature - - diff --git a/.settings/org.eclipse.cdt.core.prefs b/.settings/org.eclipse.cdt.core.prefs deleted file mode 100644 index 2222ff26..00000000 --- a/.settings/org.eclipse.cdt.core.prefs +++ /dev/null @@ -1,3 +0,0 @@ -#Fri Jan 04 09:57:26 GMT-08:00 2008 -eclipse.preferences.version=1 -indexerId=org.eclipse.cdt.core.fastIndexer diff --git a/CHANGES b/CHANGES index b02ee7d7..ea8a1e9b 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,9 @@ -07 Oct 2008 - 2.5.8 +21 Nov 2008 - 2.5.8 ------------------- + * Fixed PDF XSS issue where a non-GET request for a PDF file would crash the + Apache httpd process. Discovered by Steve Grubb at Red Hat. + * Removed an invalid "Internal error: Issuing "%s" for unspecified error." message that was logged when denying with nolog/noauditlog set and causing the request to be audited. diff --git a/apache2/msc_release.h b/apache2/msc_release.h index 0b346379..011fafc0 100644 --- a/apache2/msc_release.h +++ b/apache2/msc_release.h @@ -48,8 +48,8 @@ extern DSOLOCAL modsec_build_type_rec modsec_build_type[]; #define MODSEC_VERSION_MAJOR "2" #define MODSEC_VERSION_MINOR "5" #define MODSEC_VERSION_MAINT "8" -#define MODSEC_VERSION_TYPE "-dev" -#define MODSEC_VERSION_RELEASE "1" +#define MODSEC_VERSION_TYPE "" +#define MODSEC_VERSION_RELEASE "0" #define MODSEC_VERSION_SUFFIX MODSEC_VERSION_TYPE MODSEC_VERSION_RELEASE diff --git a/apache2/pdf_protect.c b/apache2/pdf_protect.c index d5c26482..0c708955 100644 --- a/apache2/pdf_protect.c +++ b/apache2/pdf_protect.c @@ -365,7 +365,6 @@ apr_status_t pdfp_output_filter(ap_filter_t *f, apr_bucket_brigade *bb_in) { */ int pdfp_check(modsec_rec *msr) { const char *token = NULL; - directory_config *cfg = NULL; char *uri = NULL; char *p = NULL; @@ -426,10 +425,12 @@ int pdfp_check(modsec_rec *msr) { /* Ignore request methods other than GET and HEAD if * configured to do so. + * + * TODO: Code here is only GET, not GET|HEAD as comment states */ - if ((msr->r->method_number != M_GET)&&(cfg->pdfp_only_get != 0)) { + if ((msr->r->method_number != M_GET)&&(msr->txcfg->pdfp_only_get != 0)) { if (msr->txcfg->debuglog_level >= 4) { - msr_log(msr, 4, "PdfProtect: Not intercepting a GET/HEAD request " + msr_log(msr, 4, "PdfProtect: Not intercepting request " "(method=%s/%d).", log_escape_nq(msr->mp, msr->r->method), msr->r->method_number); } diff --git a/apache2/t/action/.empty b/apache2/t/action/.empty new file mode 100644 index 00000000..e69de29b diff --git a/apache2/t/regression/misc/20-pdf-xss.t b/apache2/t/regression/misc/20-pdf-xss.t new file mode 100644 index 00000000..92d72303 --- /dev/null +++ b/apache2/t/regression/misc/20-pdf-xss.t @@ -0,0 +1,54 @@ +# PDF XSS Protection + +{ + type => "misc", + comment => "pdf-xss - GET", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + + SecPdfProtect On + SecPdfProtectMethod TokenRedirection + SecPdfProtectSecret FooBar + SecPdfProtectTimeout 10 + ), + match_log => { + debug => [ qr/PdfProtect: PDF request without a token - redirecting to/, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.pdf", + ), +}, +{ + type => "misc", + comment => "pdf-xss - POST", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + + SecPdfProtect On + SecPdfProtectMethod TokenRedirection + SecPdfProtectSecret FooBar + SecPdfProtectTimeout 10 + ), + match_log => { + -error => [ qr/exit signal/, 1 ], + debug => [ qr/PdfProtect: Not intercepting.*method=POST\/2/, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.pdf", + [ + "Content-Type" => "application/x-www-form-urlencoded", + ], + # Args + "a=1&b=2", + ), +}, diff --git a/apache2/t/regression/server_root/data/.empty b/apache2/t/regression/server_root/data/.empty new file mode 100644 index 00000000..e69de29b diff --git a/apache2/t/regression/server_root/htdocs/test.pdf b/apache2/t/regression/server_root/htdocs/test.pdf new file mode 100644 index 0000000000000000000000000000000000000000..a6ec2d04c7ff2428885770f29ac8164264ab53c7 GIT binary patch literal 15406 zcmaib1yr2PmM!k?PUBAF?rtGSaF@p2-5r8^2u^T!2=1;ygL|;xP9FUC&b66&^DXGA zI(4MZsa>^N)}~UDkYZwG=0Kq8EA0E;*VLDTzy@Fe*c)3R2nYZbKz8OX76A4)i3(84 z(#8ek1eCHdasf$zOzcfTLP7}6E>0jLTLh0RL4^r~UKXUz13D)hz9bu4Zd&N#t4&*_ z-+=Y!R`|2+<5g1@hz)xTD?I<-l48p?^}i(e_X4m2*f_aZIQ})=Qy*_# z-GSxbZSJZbF=dG;Qzu7jNKUpoEXYj&8u-@2ioImY6y>y;z zWq6wHXJnXXua5JEuSwW&gnY6fs->OgCyK#2Fm;5>OGE2zTR(+A+cIeS1tuIBe+~OG z6S$?*)q2GZH_{av&m+XbvCy^0c}pZl04b*B8=PlIs?o@gOm@J@GMFz~vc$qp9eRnW z>)#GKP*^7(%pJ&K&c9_m$2z@h$#HyJIo$e zh)=R!ssqSk=)m?*FR4Y z_K=#N8NsM;6-^~*ZT>D^qwu@zz4KTHuck8aI%3L{^EgUVFP>v?{TC5gr?6 zQDaGJ4d*<9&6-{B%`4}sye~daw!~aaoGluu1ktE;T3#b9T&018{Wg;B zD2xhpX;JBm(i6)g!;YC>zg!?Hw{~RPbmgI6{NV8+)*~E)Bh@R65Y!^W{l-9l@0S_c zHUghxLO@6OjL9J_^FWaTR}%8TA`rtzAFs=R}uaLMsH_XmV z^$d-4H!{EOx}%Y_lsManY^Bf}h)+c}pelvp`~qHsgpGDZ9_kLmx6232Rholsu4Ys= zJD^Jz`#<&!KBsuqn>j?#nGKJ7Gm5R-I2{nqn9=!)DhrHn~g9(+#tPzS8*LTAE= zR^?<-!C~wB?$!X090XGZZMlJR>SL+WiLSuOr2E@MD}LKl?<$ZlWHUH2rB z6J?*t%X?Lv@lnM)9;?EJci0F^5JBaheG-BM;R1(fs0(tECY@%9Lw#&zS*|JD)`Wiv zSnpmZY~;@z6^RnLj!}&TI}l#jlaY|2p)yfQh!%-~u~`o0sRUgH@pJ@hy@K#8xfPXX zJ?(0N+J=0x=jMV&9OOzW4a_yj-^fQ5g@n@yJYX3YLQ;39=tQo1xo3SQdL{D%?}`?t z#x)5Z8(=j4l@4Qu&))qLZt?3l{J!hH??V4=UHg(_%b7`z@dg%If=n_-T2G_cca7(o zUCC1UHn9~7@1wc7gQ!W@EN^d^iQP#bqy4Ik1GCrom2?8g*RR9TKHWzX-FtpDJ+NC| zSd15R*Ax+JUHRW%vDr0|aO}I?YG6<;Dp7z#3|rd4z;J6w-7mk*T7p_&QF|K^<92{s z#}~-=>d$IkYmnC7X#Rx4(2IA!SzG)fH+s&ugtxoc4EvD~7fwtO(v1gE8Eg$x*_p=q zT42)Qbd{+GZD5fF$BB$$*{bXDE0S*NMBND6b6%LA1fB?%82(XpegBnc%U^N%Hg z(fa5eWRDUy=-iR819y=*X?RrFiO7jeiQbfP^6P0n^eHkrMS`=|h1IgEY%~((qnJVY}0=h5*x2p zLaxXB$S!PJZC?FRFxsmBZp*#@jC$d+lP|>pu z90TY}{azn@+}i5MGx09-)A5bp_&&H2pL4d<$YGtSc806Ku&&=1(XevW1tan(%8DAuIli6`Gfw-LOZ zOFed)YJ3g|5(gSpFxE=={E_k&B>d?$wDYM+0dexCd*loKn))X#RPAs%C7LVpGVqOi zLEY0s>Q7;@fyjRNDJ*xn)+#$KsN1gu5jjYiy*f&)>S7J!FzaqTehC9h035fTia+2N ze~Qs=zFc$_5T`>;!hXG{Q=?sW#)w()nP)^bR9;pQ>Y#Sdb16_BLcqx(?cJ^Z=GHnJmIct9!Jv% z@3!Pe4NYf2Z)v+Sfn&@n?zQYYs zJBB68PeS#nyjj|VQbyrkuzJ~EJfI!~j&y>c;U^?7CHk->_So@lSja$;wZt(i+{OFy zavU}FUnA zuDNV#9rt^_;0O%=Fwv1+dNXT=?+snw136NawMCbwUwOjXoGF}hVT<|Hgzn|$-szQ2 zCFSX10Bd55v({ZUh~Nm$;uObV$61FPm+}E z7b!%}Ro--jXVUNzTrEy76fD~VKwjjj$J^Eve8UT|Z^XDc`-+fDLQ6RWv_BXl`o*f4 zFH+l1G2)VxdbEu50yE1dw92;Fv5MmM4Ant6Lj!tn_9w7amzp#DfMM>d=p12huWxVP z>Eq15*ift!Z|l4ogt4J7xJYf7ZVwhnBdHiaFUOz0-ZpVs2=UrK3A+8}aEIW)xyhO_ zGd)R=fkI*u!$^#9au?N_Ae*NrIf%haHUIkbt&okyP&>F4Bx`$zAatsvS^3AbxO5(W zHrni<9o|}Qp#?V;7f1^<{MZ-{9VYpp;Jmso=#7QAk}6uWQg5E|Ri~-g`=bLr=eo&3 z!>yq7vRjky$A(gG^%vDr6YtL>pk|kDau~Bz%Zg@cXyLD2H?|QieAd{B*sm+$wKvPBWbwOHdq6i{q11 zHRmdi6lGXWB66Hgu8YB0AE6Wx=M2&04gKX4`MIvea$!)+$M?Ta>{og))N|UGYpL5* z);DRMqi@*yNIb_Yk8TP$rw)`fuI%Cz3|gfToDBC`aIzg^u@eu+_q}`q@K-7p2X1L^ zdC-cf#vFUIwKDOKZF7Ar`UYuPh@a88&wM3XKkO6Z*h3=d8*+S1d6IK#V<8+hUV0=8 zq9V)rWJUk;4CRiFsUnyIW|ua%0xG@IA9X=i{g<1`DdGyQoV)|guYUa7=b&ewc0oaQ z{_^j-_LYpl;IE8pc1d!SD&Ap-ejhs7`q-x3ex`4z_?)#SevYkxZPUdrFMQCx_?2<6 zO2ny_leK0^=fua*k}i{y%IX5GFE)8$r8Kp%N#c(FSS{k{np>ixyjFRpTj*$YIaE$l zCOPu!=ojXX3Y{RG^7itALVBgoycv1ZSwaI=5Jz10d98*iACxT?s_JwQCmvL7w7bo# z8GrxKNM|5q)u@5X6Nr;RcrGrs#^!|y_Yn(BLa0!dz=p*FGDt+x?Joz+MOUEY46Ob- zy_M?7cRUYCll5Y~wCb2!$J9?~ZvU01CsFwI>#A`-#^eQWggm0g+ELqzOV^)*xt#5| zpTFstEb$Oc_cQz1BInZWF%HseKBY2Dup7lQz%BZ&xq9dzQLeTkQ-1BVQ1;YPcT3G4 z=8jG~Cnw)TKx_Lz{$Xm@TJ*3!Wfp=)AM-)wWu=6b;)60qx>i1FGM%QpiR@9Tn%AGY zjLfl<44BVU8U|TmuG_1VH@a&L7g5~>>{B+|NYvp40E2cM?sZp?qvTl3kzCHe*09ni zdsE=kV3cm9AcCI~A~PlQ1nt(z)v?HA2&6lIek}Zu=n#H*x^n|xI+!exp_=sQD7}Tg zUf;nwhEOxVCCTr-UG;!LHudHCP&4AgQb2JwyAGimZ&?gHeCj=VU1=dc-#`e_*b5`; ze3)D!G>2=-iF%6M*8(>@2)nMp`90>c(sY(hb;vU=31rN$;4pTWTBBK^2e2PgP3fz? znY4S1yR24|uEi!td|m3(C`%m5352E_ktQvS@;P+1tCtK6IG}O(%+Xdu_32Q_Xk1+E zAIzOx#SMPn@_NjZF-p;7rJJX^r}k+3Xf^%! zQD@Z^irb;F4f755QUp+u*)^ldAdW85U9ReqU}W~qhpnF?UI{%}R~0i^)*}Ney3A=K zRG)dL%o3!&C?3?5r;vXDpXfJxHrZuhrVnK~VNBfD|B_^qZkz_qRv$H?_%+c}HoW!> zbj-gXtfO^|^#xx^inB!r^M`_k<^+~L)-c`h#D$ir=02}dKs#bS^bRzmhLTD*7E$oi zXSQ)-JFXTozTm;dtk|zb%$_L5E2g7S{QS=YHU1yIvE&vjuN|pQ`?bZc4A7k(x#%^K zCP9(s_8K+w&yOlJYI{0oPJL}seF{c_ob~g#bumY>{+J^;v4d4)XX}Km&12gpVI|v~ zSyQNFCYH5eBc_tJ6oS5lePQ?8oazNRIb`=p!yb`uj7_-ww_aziKabJf-MZgy1!m?Up!?pvqsW#y zSWLef)~oI|4lhC%YMB7mu3ySRMY;Tr41~^g;mpT+B4M80rzCto{+v>PH+|nZtLbl( zqx)J8zH~8I)x&2JMd`;Zcnc1ZZx{92oZ|)5kXi7D^DJ=$7GOw6Sn*F+LVzzQ5ne}b z1WVUwXLa?Se;5FrW*>pDu#CZ93VkA!HWT(h%Z0iF3Yu5$y0tbG=0|NW1Lo4JVHJ-y zjX$i~s*5dE9rkUj@796L+N#-AKT4N|%>A5(Qq|V2H1g~{&$->txEY)-E_LkMe{Q(! zNqqL7Yx1t+w1k1PvjF|#XeSh?N75U#fs;u7Cb$=4_d;H=!}xm9CcXQSPV=iyGv+B{ z7yPN(S!MF>QJqzT>y9gVt7W4{V_~FqU1b&g{OM#8-E1LIU1k47RLW>d%7hv+Cj)S` zkVt8rwoBA8SYvfKy^&QC>P?9v{WA<=j4ZTMBCv5D!k2~PIB8us>>=SWWI0r4xy>tjXQ#=`tG;Tr@9No&Y zsAh&2$Hl)zx@{P5)8rs6{S;T|iPP$|>4Yc-DpeS5HD_7aj(u3~2po{P*U;BYt#_;A z_>G(eyw5%r-(GXUbx;3Nyqh~bVTOUd?#sz}oVy$ypl;_WDDzV0q(~UUVv{m%bmWp! zDZO^46$JK$xZo2+uY}m%4j6aAtBuj~=xXK7XEab}nFSY0saTVT!CnPLii+JX<>!-c zhp{@8*t*n*PAe4$dj#;W(Zal5OVBY+aHsI`mKH6mTE5bw;K=KWOV%ZUQ$blO9!J!0op(BqC^$1?!Z#F_uZYz2Sir&94V5A~rke3)h$8K@Ih-H6CRPtzVsv`=Q6U=5jw8f4Fn52l87x z3#`3j>+M3NW;`OQGI*ij&m2o9@ZI?3>hGQR>LID7H4FRtPddI_RFr1jITylS{yx+F znQx`+UfMQKir`(x5cTyB<+;YT|p_$xnCwRID~Hud#No3{h)! z4@n_U@+LmlD7`6aW$IBA0tntVW^r>x?q8EWU$GAysB18jgc%Gkbt2dq2-MYv8IfzE zleW>*%7fJX^M*Byl6Qw!*{jBXjjr3=i&bIOJ4JqCR*k%2plZ+1q2w;VwXh%e4&XKQ zVB6u`|1hDmQa`@2ghcF7tibSNw5@2Zj+STF?!kFB$6``Ojhs1)04j?Se$-8a*Zqe)Su?EZ#|Ws~%5Aeurv*^e^#ihN{( zv>)UoXTRnvd`c{$`EGXj+{XOE`~t3&BU8=YN#C1aIv+Wr&=s*EEJ4XwIsG76B)5kx zW#jTqz6dx{rz_P(jVN8`209%xuAekBiyd=_v=JLj9=;t}PpH+_5`mGArvlM{FqR7} zz0g-aWOLVL2UJlq#V=$I=5yE*qzz`v_F0Bt^1`uK_WbO&f3S<-v!1jq(D9=&EhcNR z7~vu<=SQx>mA}Kqw2wRNS+$fQcO3m_Z&p?wY01$bOeUIc)Z{uHAR)v-NhGO~u3&jr z4x!&gAR^@^nPxC*j**D}keFbPDTmNmxGC?XjZir9;$<5v94>!X&fCV>i@X+umq504 z*KA{)t^f)}IfJ}bWawC}lU?ui_2stjoUxwybDa(Q=Q^#p^R~d5dq=u|C2L*sxY``2 zD_Q1RKc)3aFJ|KzGqv;H;zN4@p9Asm49s1wofqn_9_u)A(i$h}51?B6`FIv;g~gWq z&g#rUldyB%*j7K==f)$ZaLQtlW9Pl6udhJ^RTvxlK@Otx+-It+3rr;3${~A-YqVUe=**B#b@CuFu|;t{}4Zi=$FWGv605igZX*4LTs;1QqR#S zaC{Otgn~&4%`3ULWZ95*MZ@W&F3zwz2%%j+VO0ynSswpsY`Vt?m!t{#D~PaBv4JV< zXpKN3DYUuUA<3{=Lwl?)q&*^N`3e#K_Fm`PGbi>zHuJ{eI0sGz-H^ZMFI>#7S6R!7 zPA9$f%{2x)3kG{oCl5QH-38CrAE{9M5;glH<)nNGO}cq4{@-8D_K~KGiDRcta;F$g zC4sP=xg7LYcC&tJPx6%>?QY)t6D!_qx5ox7o$+^>>Si*Fp#2Q3Qj(3VClLEiqAa77 zX`?KYRO63q+lkti+1Yso6bq;kg+?a67#D5gVul%L7vnX#_T0n(VrMs^M$(3yhI%(^ zt`nWR9j>cp%&1 zOc?8G$6!D!qgXg+U4&r`9)VtDjn0deYVTs2A>tE*n6^2V?S0akF=KGPaxcXE<>4V? z!qGg1a}worTVPU`ytdXRNeZz<}da5s<|78QLJD^Bg7-;#H zt^qk26y!tAprgL&CK!gmO3z4#ge8S28H$M!DH)|xc+Znn{q1)WIJ8`S#?{%?L=r!| z$*yVR=})@#!#^FW-c>j!B*sMkg+c8E!k%uF1;Q1(f)M`Ry`vB9}3*lwsRq?KiMb zkzLfi1M7MV@FOAD0Ve^^LZ`k*vemi_GjbVK`OMrViA91&scUw9AJdCeD;N!Hzo4ea zeZhBb>&y7OR_TDoOh-RoBrl?)y%h27cN@ji=5PK1AA!i|pQ@UKO6>@>ib+)+dIdcw zb)`f{XH~hLACPkve_XT{B<}|@CN&z9BPUgsFfe6ueRU)MkeIr|J1D0~w;k8@d{zr% z$2c|HC(C4d?wY-w!#}@}9sOhBYp{uZ?Ir=Axu}_Xr)tjmxKoCb=(k`2v~5z@0l+xe zI4a97T}VeA(PY$h&9F3HJ_R8W`pur#bNooF=}YSdPFw1}f?|_!E7MPXRijB}15oYm z6T~;vkcH^`4TaXXkNTUgZ-h3tKTUvw@3gRbU;uFwJ5F7q&8SXD{m# zw=TCCHe6pujkTahAUR(Luz9>y)|yK=31RDW>f7rJmNA!M1C`3?Ipj5#9rCnP45t{C z7R}Sy32)$JsSrYltx{N!{LNKcTa3j*b<|pPu+=`4pp;FP^p}lPc%s_g7hAeE9+1v; zqsKMWRbwUIMNZU$_i(r9=ved{SvamCR0Nz!XLC<$h=wr8UJ9t5M_y zByxof(24O2L}fuHuasfv6jz~KqeB$!eyZI@DU#5VGM~$(ipXIZJHzkBPC0)@4wZgoX#EPO}8At5O15cR(5A1;l>b(3EE+>B0far>L~Jld0}9fg&+32l^E2`RKPaF^(jON5Z1 z@SXrmlmZMsFiIR*DGFS17BC-92Mx<9KD_~}Nl}UVN*en~;O!fi1@SR7g0BUUbAN{M z(DxZ6{aWPWHM&PFYz43Jf_#Xt@xe$i8W#ERfGB9HrNY`{FXoat1!W- z53qCUfus^J!a4o=u;Z6z_4%+6btR1MTqiHq;YN6o2DVXxz$l$ll$Yl6AXmWxqql@G zKpf?s@@2$dG4WwRWCgE0={e5J_?5(1v^jrZX(TQ|GIG!Fh{trk_LOS1|1!oJ+6w-uUI6chT8K<%$(Us`5 zv5Ms2lJMB^2}k#JAUr86yLZAg!^!Zz#BeISP>{u}$Wvj4m*;rlbAK%E_{7%Z$r^lMt4{uX0j4A? zsKI^%Vr}T?=Xa3fCt+VK!Xgz!vXIM4BE=mxk&zUD=vIx}trzRgn)PzkOI>HJ*9 zd6mvw?s@(fPWJ~sF$QZYR=nT!r1$R+P7~AmBG9IA<+7Svh01)_+oVfF9ulOERvvhD zjxoA&WC?(f4V9_ zd=7Vz7qCl}Hby3Dl+!0QUF+ajK%TJlkclV|1QSngXEhoq;8?h%IL&1Wc4>gq^%+0G zc40^I_0Y11XcOY=O9vD@Toi`iuF?M|JPIb?|2m-(Bg$_+bB4eFZeo60+I=W*W?# zFsNCC=^`7%;T8W#U803b$NRz)^NXaDu|FBg_NDk$|R)n~>c1 zw@VYOF0aW2tv8t!Y~VF6F7tXHxx-E7AnZ#oYx#Nj6J$4sLzi;@PUR$zAt}@iIm{l_ z!K=MOG2+_(x*B@FAzt9Fxzu{^`I=yAHf72mLY#K5;-Qa}MY`wt@fU8zp7CWA+%Y)@ zB2z?Rz0lM0!()gHMnSePA1|A-H+{R$;0 zN2NGOnx%s}wdiw9f|E}P^j|N}5Aw~5`-W(Hc%|;LE~Gx&=&$Zv?@i`k7d@qe+%k88@vR4DJ0CMWe}3R_ZZRpWnfg*X|84JU$7wSZ?o+AE<=F%71sdzi6UG&nn zP^`Orb268}Nz&bGm1b{|YB69ee0$S|U%jC&+_*m@L)B2=O*a@F)~k_5gqJVOxP>YRGea$!tQj zAl+Iq?&+L%U};K;zV<)kYt^&>5Pb|W&J$u5=%os9>GP`MOy9h0ZQ|6G_wR~BjJnS;OF_U(!Wj1)ZZ18Sp)KMep25W-#djJhg{A4_z{Mhk4hDp4@{r(PI1u)A zM9ReI5XanvN?`yNF6)|l9Ed!~7-7A`#B`0-(_XeT1$v+hW|ZE(hm23kVvIK6sc>{P zEksr4z*s?#gDJ>M4?hmacS8#EhHZ5ST1Ag>_RaI~Fr=J6*l9_Dw?nhqE)cw!Zc^Sf zvWPi`zmW;f)yL;RZ8wP$eavMYiQ>y(APZ!hr}mVqiO15 zh<4o@S|t}EwY_yprfC{sfn8(4D5eRGZX1VWOOts>`dRcGPitD8yj9B=92Iv4PobFu z4^~Th_#Pm?X#{nmYe+=7ICK-#BJGVS!a+bIs3Tr$LCZ=|*fh(BMI;*{I%T9kZkS-A zYg!T)O*1Ic*b&^LiT2oQ5$Rx`b&S7vgL#4|H4+gC$1Spe-d4uV!6AVo0L8U}NrFH# zXt+Idh=z6wM+4IuCnvpgcS z8xlh);y2C;?h)}`noLBn_yru*G2&2xcn@o5oxL}0DqK?IBMfGw#RtcWSYm#kz94|F zA>Tl78 z?xkwu#l&j#e zW1+qC5S~e}R5!tzlULBQiV`ql-xDwwJa*u@p}dWhBgm+%FyWRRWyY@To1EiF?jJ2@_1K3@o!#ed9EEr=pm4QGT}^Jc>z{$}BE~7#!_fN$%uHgqeRygs zxSp_^o0`&=GKMmQ*0NbEds#`@NZrmne0q5~c?NnwC4KdF8IIZ`!;95`@b9)MS1A>G zoTUOkIPNiYuP?H8h{GSY@zncfOc04gg>qrjmYbHt{e7Q<`ERXHjBd4VdAgNv@t&57 za2e9{D|EeTosI?&+OCcFW}{Q&f$Gd1nyvrq;KBcLU^J+h^xy(e)TO!Fj(ZKyQx4W}N5OpP)(y>^cW9y|FHTDImGt2?LU~!NU@h~cgBCH~arADZ=;Q>>yBVBdk+DK^NORxYj z+miyNu9U(PiW3~u;Lhe6cXGj9?`3|ASEgZSvOm=GU&y*hM=6B?$>*V24gMcWHcESoHjC#EOClfh~N*5H@E0^0Z^gUAL> zZcfm?9(T*PtQ^Xm0 zS{IUIG2oNj63;j;jR7d&6Jj@b4d9Mv`0G3k*;g1I(PR4n7l=&&T`j*zY-slnJGZgI zF$gIxhkjDpa_`JjSY_CyR|;NF(9>kvE`o7Hd@+1$*mRhM^%sYb6N-3O?qt*Y1=Q?IL{ zDwHQsqCj7qLX*5*XsYcb=A_~zMZQl-l^9T{Hp>(#H%Nt<7>vRDAcr$o8ACsmQlUOV zC!Y)sDp#>d*bctot`poXM~b%s&j-TWqdcfkD7aIj{fT(FWqXg4LF08ku67%K`2x-d6z8xh%YgOCKqod1&8ujbMb&^kGEjZuU zyS9Ew?KyYYxS5BEc`P{Jx17|#*XL%eTvD%`qTIdGtbF*clzNQ{#L>(v`?_nYXs4C( z?UilcQBx7O5Eg75+%Vy9a3&Uk5*ZgzwC;jz+z4Db&&3H~24lT`M1g0G*AC%Q@egQ1 zk7-~>T+z~eHc8^#W4b9PJ8=Ui0y8u^8OnWOm76e$n|q`zq_Omk7V(c?z@$g)wh_jF z;+}J8#{cQSUIuvx5dje~Gj>9KjRF>fQVI<~_{|d#v4*rr?S^?p+(KePVj>9cTDl%o zS7-P%T{PBkqb&v9lsgzVeNB>i_RW&`194!3+OIf`(ylH!-#L0$a6k+q7Z+z|(zk_x zI{g=kj>1!FEmFodm*@-);iC=&I(%Lrxt7{YxkXHhg$BGr7?0CGqp`968if530~a7 zJ0XcIs{+SN7n}lBv@`4cC&v+42f+xGYKzgVK7tnb)+N4;bfpX5Sm9S@Hq_-KG-}gg z1;T`+eM?R83=Imx2~g7Qu_4w7=A>$B*P_q|oLHgW%#kO?E^nBHX7s2(h}BsFrk;DW z=!=DWpeZajA<#qXi{%(9v8u;TA!vo2DmZK>n+`_Uk{3x#5rQc=r%vDuzs6@sQHQia zoT21Vq;k=z+P5Cmg*lp(dMq%7#x>1S;X%RS?vbjgzDQEX=&WdDfimA~3LaWCNK${F z*XD&#E#Ne)@++OZBDotcJi897C+5LzbTq8YM1d-CdnQ0SGyYbFTMn4UoSG`%p`w^c z!zs%S;>%D^rB)rmV#`n<#~i`KPXs0A2s_Rhz$I85ePoW7q0etgC8HDX(htXDH0gFv z#J6Fe`|d}l_$yP{d3Dw_rQK65Q#-A~jhhBkX<#87b74!YB*$iu-CI&gjFkLo&4N<6 z$rH;+*Jr0vD$&4sr;?bsg_n0NI@7ylcj(xNIBJZWX5h96Cb_X=QmI%c@yuYsBcV`7 z=HO*8$f&2f4(~G2owM5hqfq8z0waYCeRTnEYu2;EfwCDPrlpm@sD$joj31W4gpIYf z^skE_pNXAKhX*ABk$xn`#1o5hiG|tR!%^vc80l89{qQ;5mMip$hn>F=O zT%>#(Htv@vE&!UZLy`Q40(d#aju#w;&=ANJkeO32dd2=XnA(p(v>l3`jq964Qu)&+ z?ohC{B=2Tag2Wv`BKMYa?%wK>efPU-ltckMxz|AwD}~!-!q&c?^=OW}#Qofk3zlED zR@TBl3E?;uVIX^`y=!WBhQ-?nGI7Tnz3r;P^lj;k<*>V z?U@;rk_F=C4jYp;8}wD?RiRbz)g3N)%a;6?Qjt^e)g0RsT1`yF?&8vfNnQlr=LBP8 z4n-k2>Ilf+bFI>A$fq}4^N7XzNHi|MQLq}wWXK9wv;zO&Yqfw4@MmO^kgD)&lmJ+; z*m#14F|RL3TW>#2hkt=0hSYH@EUc^WLbx;(&Z*uQKlot%@V%*X9PGhVuQgx4FVX%Z zY4CM1m_7J2`eqkczYV^Q6fEgpWfMj0wjI(*l7WwGJms0<^oDhn(G`5V=Tm51veWFPK-l6^jT zI)H#u_I553AZHUNO9vPGH#XDTP|?U11QZi}e;7Vl+Jc-FLGCK{wnlbJsvkZfyc3?x zodFzgoi|-EF?$akCQdFM024b47l4h0jT^wq!osBolyxz(u{062Gq(W&SP+1s&L$u` z7XS}0D+2I6-ur-wjg<=lC~o8+1F|%?aQVv+0jTN%vQ-E0yxF`5cwbBi-#J-qJly{WBTFw~!Y%}W6?!KCJ$7VF&c`Pl$(q5(9vS^w*o2zv&`605 zx82(e)cs~V3%%RE?p);{^J{;W_9#)K+!HYtMFk$QUY`+mrH>X{mYtr`{SLuh$Tf^| zeYL7SxKufqSw8Xm!}`gI9JhI%w+CP(8r!gl;Ap?V&UcM@(5DA#R9UJl3IXpqt9Vkt z9h35Kg{!x@NT+%IlE%XTc^c`MgCx`%y#9!1N%6CY+#tR7x*aw+)$7fyfc9gczuSVm zDiBmL%rJ$2`HJU(`}U?qTId7dt4^RZ!#-~Nmf4~M9#L)d`^P%ynxg?Y*hit z14b)~&*wjF+mdHAC>jVEetlQ&i#;2kU~2>*hfw^09}^hvP--V^=zdDefKf6MI+n187Nm7MHNR6#B} zz&Dm4@Ds?xMepC-{?qEO(kL03gMdm#PVZ3Xcy|C)0Xf^dI+=i+0j%#KRDd5qrj|zU zN9a8O7b_cpo0m-wDD7nL>hMN4RDCP6k)5-{y9*Ofpt$PW$#JtZ0jWrf0c8O$Z(ATF z1QdT;;9CS|z&lUzU(d*U$oKRA=I9^GaYp!WYe4wtBHj$%Hi4HDz{A1zURZfcQ)eB( zUn_kdirc%s*WzCb`7hkazha0Ry`6e{^S7+c-;(?*X$0U$2auhpiHoJZozCBU0X4O> z0d}r7Hh+6zdGoDcWM@tbvSU(HrTe#%{uf{IEeY{AX64%zB_Jg!CB?$R$<4yT&GvTF z@V>PiZ|xg+xY>jd-uKAV)dcikdK_=MZ>oRIb4jwWypOrK{{H`F%f4?oUBYN+%zmKG<5$`8xJRt z83GI74d!p2{{8^CI62ul0cL=|$yhnqdEaj7_XlA2ciCH?^$n2!A!B2EtL4ARSUEV^ z{|7ye|E9;z$@+GC{<|$ZC;Qte`41WQ+sgh!#=*k-wqyUU$HC6>pK;&v;P~%)T}t+EryGe(c9(j^bSx}OE1t{semf>_HPLL3&{vTSvxa(0PA05+qZ!PKnEbeDE!6hmx z!Og=hDkUx|Df+hMtP)&2;-V}P;#?A}JW}s~bapXva`|gZSvh#w*bu0wBtJ?a{6Avz B`Q`us literal 0 HcmV?d00001 diff --git a/apache2/t/regression/server_root/logs/audit/.empty b/apache2/t/regression/server_root/logs/audit/.empty new file mode 100644 index 00000000..e69de29b diff --git a/apache2/t/regression/server_root/logs/subdir/.empty b/apache2/t/regression/server_root/logs/subdir/.empty new file mode 100644 index 00000000..e69de29b diff --git a/apache2/t/regression/server_root/tmp/.empty b/apache2/t/regression/server_root/tmp/.empty new file mode 100644 index 00000000..e69de29b diff --git a/apache2/t/regression/server_root/upload/.empty b/apache2/t/regression/server_root/upload/.empty new file mode 100644 index 00000000..e69de29b diff --git a/doc/modsecurity2-apache-reference.xml b/doc/modsecurity2-apache-reference.xml index f9fac9cb..0afbbfd4 100644 --- a/doc/modsecurity2-apache-reference.xml +++ b/doc/modsecurity2-apache-reference.xml @@ -6,7 +6,7 @@ Manual - Version 2.5.8-dev1 (October 6, 2008) + Version 2.5.8 (November 21, 2008) 2004-2008 diff --git a/review/pre-2.5-brian.review b/review/pre-2.5-brian.review deleted file mode 100644 index 0d2e9b5a..00000000 --- a/review/pre-2.5-brian.review +++ /dev/null @@ -1,3436 +0,0 @@ - - - - - 2008-01-04 :: 10:55:40:361 GMT-08:00 - 2008-01-11 :: 16:47:15:701 GMT-08:00 - - brian - brian - apache2/re_variables.c - item.type.label.suggestion - item.severity.label.normal - Is ENV really cacheable? It could change via setenv. - /* ENV */ - msre_engine_variable_register(engine, - "ENV", - VAR_LIST, - 0, 1, - var_env_validate, - var_env_generate, - VAR_CACHE, - PHASE_REQUEST_HEADERS - ); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 10:57:54:279 GMT-08:00 - 2008-01-11 :: 16:43:38:825 GMT-08:00 - - brian - brian - apache2/re_variables.c - item.type.label.suggestion - item.severity.label.trivial - GEO is probably not cacheable as it changes with every @geoLookup operator. - /* GEO */ - msre_engine_variable_register(engine, - "GEO", - VAR_LIST, - 1, 1, - var_generic_list_validate, - var_geo_generate, - VAR_CACHE, - PHASE_REQUEST_HEADERS - ); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 11:02:30:450 GMT-08:00 - 2008-01-11 :: 16:43:20:652 GMT-08:00 - - brian - brian - apache2/re_variables.c - item.type.label.suggestion - item.severity.label.trivial - GLOBAL is not documented. Is it cacheable? - /* GLOBAL */ - msre_engine_variable_register(engine, - "GLOBAL", - VAR_LIST, - 1, 1, - var_generic_list_validate, - var_global_generate, - VAR_CACHE, - PHASE_REQUEST_HEADERS - ); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 11:06:50:690 GMT-08:00 - 2008-01-11 :: 16:43:02:916 GMT-08:00 - - brian - brian - apache2/re_variables.c - item.type.label.suggestion - item.severity.label.trivial - IP undocumented. Probably not cacheable as it can change via setvar, etc. - /* IP */ - msre_engine_variable_register(engine, - "IP", - VAR_LIST, - 1, 1, - var_generic_list_validate, - var_ip_generate, - VAR_CACHE, - PHASE_REQUEST_HEADERS - ); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 11:08:36:733 GMT-08:00 - 2008-01-11 :: 16:42:44:589 GMT-08:00 - - brian - brian - apache2/re_variables.c - item.type.label.suggestion - item.severity.label.trivial - RESOURCE is undocumented. Probably not cacheable as it is easily changed. - /* RESOURCE */ - msre_engine_variable_register(engine, - "RESOURCE", - VAR_LIST, - 1, 1, - var_generic_list_validate, - var_resource_generate, - VAR_CACHE, - PHASE_REQUEST_HEADERS - ); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 11:14:32:043 GMT-08:00 - 2008-01-11 :: 16:41:31:558 GMT-08:00 - - brian - brian - apache2/re_variables.c - item.type.label.suggestion - item.severity.label.trivial - SESSION is probably not cacheable since it is modifyable via setvar. - /* SESSION */ - msre_engine_variable_register(engine, - "SESSION", - VAR_LIST, - 1, 1, - var_generic_list_validate, - var_session_generate, - VAR_CACHE, - PHASE_REQUEST_HEADERS - ); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 11:25:25:879 GMT-08:00 - 2008-01-11 :: 17:00:23:571 GMT-08:00 - - brian - brian - apache2/apache2_util.c - item.type.label.suggestion - item.severity.label.trivial - Portable way to format sizeof()? - msr_log(msr, 1, "Exec: Unable to allocate %lu bytes.", (unsigned long)sizeof(*procnew)); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 11:48:52:563 GMT-08:00 - 2008-01-04 :: 11:50:30:473 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.irrelevant - item.severity.label.trivial - This #if 0'd out code should be removed. - /* Removed %0-9 macros as it messes up urlEncoding in the match - * where having '%0a' will be treated as %{TX.0}a, which is incorrect. - * */ -#if 0 - else if ((*(p + 1) >= '0')&&(*(p + 1) <= '9')) { - /* Special case for regex captures. */ - var_name = "TX"; - var_value = apr_pstrmemdup(mptmp, p + 1, 1); - next_text_start = p + 2; - } -#endif - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 11:53:45:291 GMT-08:00 - 2008-01-04 :: 14:51:42:573 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.optimization - item.severity.label.trivial - Use apr_array_pstrcat(msr->mp, arr, NULL) instead? - /* If there's more than one member of the array that - * means there was at least one macro present. Combine - * text parts into a single string now. - */ - if (arr->nelts > 1) { - /* Figure out the required size for the string. */ - var->value_len = 0; - for(i = 0; i < arr->nelts; i++) { - part = ((msc_string **)arr->elts)[i]; - var->value_len += part->value_len; - } - - /* Allocate the string. */ - var->value = apr_palloc(msr->mp, var->value_len + 1); - if (var->value == NULL) return -1; - - /* Combine the parts. */ - offset = 0; - for(i = 0; i < arr->nelts; i++) { - part = ((msc_string **)arr->elts)[i]; - memcpy((char *)(var->value + offset), part->value, part->value_len); - offset += part->value_len; - } - var->value[offset] = '\0'; - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 12:00:50:877 GMT-08:00 - 2008-01-04 :: 12:03:50:156 GMT-08:00 - - brian - brian - apache2/re_operators.c - Suggestion - item.severity.label.trivial - Use resolve_relative_path() instead? Maybe a config_relative_path() to just get the path? - /* Get the path of the rule filename to use as a base */ - rulefile_path = apr_pstrndup(rule->ruleset->mp, rule->filename, strlen(rule->filename) - strlen(apr_filepath_name_get(rule->filename))); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 13:23:49:834 GMT-08:00 - 2008-01-11 :: 16:26:24:257 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.clarity - item.severity.label.trivial - Add parens for clarity. - *next++ = '\0'; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 14:54:09:456 GMT-08:00 - 2008-01-04 :: 14:55:05:945 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.trivial - Need to check return code and log an error on failure. - acmp_add_pattern(p, buf, NULL, NULL, strlen(buf)); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 14:55:53:308 GMT-08:00 - 2008-01-04 :: 14:56:17:267 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.trivial - Need to check return code and log an error on failure. - acmp_prepare(p); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 14:58:08:006 GMT-08:00 - 2008-01-04 :: 15:00:17:190 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.optimization - item.severity.label.minor - See if apr_strmatch is faster. - msre_op_within_execute - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 14:59:32:341 GMT-08:00 - 2008-01-04 :: 14:59:59:858 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.optimization - item.severity.label.minor - See if apr_strmatch is faster. - msre_op_contains_execute - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 15:01:39:208 GMT-08:00 - 2008-01-04 :: 15:02:04:258 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.optimization - item.severity.label.minor - See if apr_strmatch is faster. - msre_op_containsWord_execute - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 15:03:12:792 GMT-08:00 - 2008-01-04 :: 15:04:40:965 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.optimization - item.severity.label.minor - This implementation comment needs to be coded as many string operators now attempt to resolve macros. - /* IMP1 Duplicate the string and create the array on - * demand, thus not having to do it if there are - * no macros in the input data. - */ - - data = apr_pstrdup(mptmp, var->value); /* IMP1 Are we modifying data anywhere? */ - arr = apr_array_make(mptmp, 16, sizeof(msc_string *)); - if ((data == NULL)||(arr == NULL)) return -1; - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 15:12:55:300 GMT-08:00 - 2008-01-04 :: 15:13:19:677 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.minor - Need more unit tests for operators. Start with new operators. - - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 15:36:54:292 GMT-08:00 - 2008-01-04 :: 15:37:50:111 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.trivial - @m operator is not documented. This does the same as @contains, so it was suggested earlier to use the @m algorithm for contains (if faster) and drop @m. - /* m */ - msre_engine_op_register(engine, - "m", - msre_op_m_param_init, - msre_op_m_execute - ); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 15:43:40:931 GMT-08:00 - 2008-01-11 :: 16:38:59:940 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.trivial - @geoLookup should set error_msg on success to something like "Successful geograpical lookup of \"%s\" at %s." - msre_op_geoLookup_execute - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 15:47:09:574 GMT-08:00 - 2008-01-11 :: 16:40:07:483 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.trivial - @rbl fails to set the var name in error_msg. Should append "at %s". - rc = apr_sockaddr_info_get(&sa, name_to_check, - APR_UNSPEC/*msr->r->connection->remote_addr->family*/, 0, 0, msr->mp); - if (rc == APR_SUCCESS) { - *error_msg = apr_psprintf(msr->r->pool, "RBL lookup of %s succeeded.", - log_escape_nq(msr->mp, name_to_check)); - return 1; /* Match. */ - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 15:49:05:878 GMT-08:00 - 2008-01-11 :: 14:56:55:081 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.suggestion - item.severity.label.major - Change from TODO to ENH. - - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 15:54:24:055 GMT-08:00 - 2008-01-11 :: 14:57:08:520 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.suggestion - item.severity.label.trivial - Need to remove the LUA #ifdef's - #ifdef WITH_LUA - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 15:56:23:415 GMT-08:00 - 2008-01-11 :: 14:57:21:641 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.suggestion - item.severity.label.trivial - The LUA #ifdef's should be removed, but if it is decided not to, then this lua call needs to be #ifdef'd. - } else { - /* Execute internally, as Lua script. */ - char *target = apr_pstrmemdup(msr->mp, var->value, var->value_len); - msc_script *script = (msc_script *)rule->op_param_data; - int rc; - - rc = lua_execute(script, target, msr, rule, error_msg); - if (rc < 0) { - /* Error. */ - return -1; - } - - return rc; - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 15:58:01:918 GMT-08:00 - 2008-01-04 :: 15:58:58:621 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.major - Need an error_msg set for lua execution error. - rc = lua_execute(script, target, msr, rule, error_msg); - if (rc < 0) { - /* Error. */ - return -1; - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:00:09:204 GMT-08:00 - 2008-01-11 :: 16:38:08:350 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.trivial - @validateByteRange does not output the VAR name on match. Need to append " at %s." - msre_op_validateByteRange_execute - - - item.resolution.label.invalidWontfix - item.status.label.closed - - - - 2008-01-04 :: 16:02:02:403 GMT-08:00 - 2008-01-04 :: 16:06:22:410 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.trivial - @validateurlEncoding does not output VAR name nor offset in error_msg on match. - msre_op_validateUrlEncoding_execute - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:03:51:127 GMT-08:00 - 2008-01-11 :: 16:30:07:550 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.trivial - Numeric operators (@eq, etc) do not output VAR name on match. - msre_op_eq_execute -msre_op_gt_execute -msre_op_lt_execute -msre_op_ge_execute -msre_op_le_execute - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 16:12:30:943 GMT-08:00 - 2008-01-11 :: 14:57:45:711 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - No. - /* ENH Do we want to support %{DIGIT} as well? */ - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 16:14:51:515 GMT-08:00 - 2008-01-07 :: 14:22:10:119 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.missing - item.severity.label.trivial - Implement. Need to check if Apache will return an invalid status code - /* status */ -static char *msre_action_status_validate(msre_engine *engine, msre_action *action) { - /* ENH action->param must be a valid HTTP status code. */ - return NULL; -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:15:55:149 GMT-08:00 - 2008-01-04 :: 16:16:52:467 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.missing - item.severity.label.trivial - Implement. - /* pause */ -static char *msre_action_pause_validate(msre_engine *engine, msre_action *action) { - /* ENH Validate a positive number. */ - return NULL; -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:17:34:464 GMT-08:00 - 2008-01-04 :: 16:22:35:501 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.missing - item.severity.label.trivial - Implement as a valid URI check with apr_uri_parse()? - /* redirect */ - -static char *msre_action_redirect_validate(msre_engine *engine, msre_action *action) { - /* ENH Add validation. */ - return NULL; -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:22:43:274 GMT-08:00 - 2008-01-04 :: 16:22:54:679 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.missing - item.severity.label.trivial - Implement as a valid URI check with apr_uri_parse()? - /* proxy */ - -static char *msre_action_proxy_validate(msre_engine *engine, msre_action *action) { - /* ENH Add validation. */ - return NULL; -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:24:58:322 GMT-08:00 - 2008-01-11 :: 16:13:48:178 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.irrelevant - item.severity.label.trivial - I believe this is already done and comment needs removed. - // TODO: Need to keep track of skipAfter IDs so we can insert placeholders after - // we get to the real rule with that ID. - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 16:26:46:732 GMT-08:00 - 2008-01-04 :: 16:27:54:881 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.irrelevant - item.severity.label.trivial - I do not see a need to validate beyound what is already done in the init function. - msre_action_skip_validate -msre_action_skipAfter_validate - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:28:48:332 GMT-08:00 - 2008-01-04 :: 16:29:03:587 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.missing - item.severity.label.trivial - Implement. - /* phase */ - -static char *msre_action_phase_validate(msre_engine *engine, msre_action *action) { - /* ENH Add validation. */ - return NULL; -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:51:52:029 GMT-08:00 - 2008-01-04 :: 17:03:32:872 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - Probably should also calc length and validate a length > 0 instead of just checking NULL. Other checks would benefit from checking a length as well, so no harm in calculating that. - if (value == NULL) { - return apr_psprintf(engine->mp, "Missing ctl value for name: %s", name); - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:56:10:995 GMT-08:00 - 2008-01-04 :: 16:59:47:800 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.irrelevant - item.severity.label.trivial - Why register init() if we do not use it? - static apr_status_t msre_action_ctl_init(msre_engine *engine, msre_actionset *actionset, - msre_action *action) -{ - /* Do nothing. */ - return 1; -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 17:00:49:340 GMT-08:00 - 2008-01-04 :: 17:02:15:141 GMT-08:00 - - brian - brian - apache2/msc_logging.c - item.type.label.programLogic - item.severity.label.trivial - This allows an empty string as a valid part. This misvalidates "ctl:auditLogParts=+", etc. - is_valid_parts_specification - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 17:04:51:155 GMT-08:00 - 2008-01-11 :: 16:12:33:664 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.optimization - item.severity.label.trivial - Inner if's should be else if's. - if (strcasecmp(name, "ruleEngine") == 0) { - if (strcasecmp(value, "on") == 0) { - msr->txcfg->is_enabled = MODSEC_ENABLED; - msr->usercfg->is_enabled = MODSEC_ENABLED; - } - - if (strcasecmp(value, "off") == 0) { - msr->txcfg->is_enabled = MODSEC_DISABLED; - msr->usercfg->is_enabled = MODSEC_DISABLED; - } - - if (strcasecmp(value, "detectiononly") == 0) { - msr->txcfg->is_enabled = MODSEC_DETECTION_ONLY; - msr->usercfg->is_enabled = MODSEC_DETECTION_ONLY; - } - - return 1; - } else - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 17:05:44:757 GMT-08:00 - 2008-01-11 :: 16:12:12:876 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.optimization - item.severity.label.trivial - TODO needs looked into. - if (strcasecmp(name, "auditEngine") == 0) { - if (strcasecmp(value, "on") == 0) { - msr->txcfg->auditlog_flag = AUDITLOG_ON; - msr->usercfg->auditlog_flag = AUDITLOG_ON; - } - - if (strcasecmp(value, "off") == 0) { - msr->txcfg->auditlog_flag = AUDITLOG_OFF; - msr->usercfg->auditlog_flag = AUDITLOG_OFF; - } - - if (strcasecmp(value, "relevantonly") == 0) { - msr->txcfg->auditlog_flag = AUDITLOG_RELEVANT; - msr->usercfg->auditlog_flag = AUDITLOG_RELEVANT; - } - - msr_log(msr, 4, "Ctl: Set auditEngine to %d.", msr->txcfg->auditlog_flag); // TODO - - return 1; - } else - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 17:08:00:396 GMT-08:00 - 2008-01-11 :: 14:59:05:782 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - That warning quieter should be s++. An evil typo that was fixed in 2.1.x, but not trunk! - while(*s != '\0') { - if (*s != c) { - *d++ = *s++; - } else { - (*s)++; /* parens quiet compiler warning */ - } - } - *d = '\0'; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 17:13:11:886 GMT-08:00 - 2008-01-04 :: 17:13:40:681 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.missing - item.severity.label.trivial - Should log an internal error here. - else { - /* ENH Should never happen, but log if it does. */ - return -1; - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 11:14:40:912 GMT-08:00 - 2008-01-07 :: 11:15:54:834 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - Should log a level 9 msg here. - } else { - /* We could not identify a valid macro so add it as text. */ - part = (msc_string *)apr_pcalloc(mptmp, sizeof(msc_string)); - if (part == NULL) return -1; - part->value_len = p - text_start + 1; /* len(text)+len("%") */ - part->value = apr_pstrmemdup(mptmp, text_start, part->value_len); - *(msc_string **)apr_array_push(arr) = part; - - next_text_start = p + 1; - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 11:37:09:009 GMT-08:00 - 2008-01-07 :: 11:41:55:614 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - Probably should use apr_strtoi64 where we can tell if there was an error in conversion since we are potentially taking a value from a macro expansion. Also may want to look for overflow. - value += atoi(var_value); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 11:43:13:789 GMT-08:00 - 2008-01-11 :: 16:10:24:140 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.missing - item.severity.label.trivial - Missing error log needs implemented. - } else { - /* ENH Log warning detected variable name but no collection. */ - return 0; - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 11:52:38:857 GMT-08:00 - 2008-01-07 :: 11:53:56:791 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - Not sure why we would not want to deprecate a TX var. Further rules could use this even if TX is not persisted. - /* IMP1 Add message TX variables cannot deprecate in value. */ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 11:54:14:673 GMT-08:00 - 2008-01-11 :: 15:04:13:383 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - Missing error log needs implemented. - } else { - /* ENH Log warning detected variable name but no collection. */ - return 0; - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 12:03:17:626 GMT-08:00 - 2008-01-07 :: 23:10:15:221 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - The timeout is hardcoded to 3600. The docs state TIMEOUT is read-only, but this is not true. So, you can modify TIMEOUT. - /* IMP1 Is the timeout hard-coded to 3600? */ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 14:12:19:250 GMT-08:00 - 2008-01-07 :: 14:14:28:669 GMT-08:00 - - brian - brian - apache2/msc_logging.c - item.type.label.suggestion - item.severity.label.trivial - apr_dir_make_recursive will attempt to create the dir straight away and if that fails keep backing off a dir until it can start creating, so I see no need to cache. Besides, what happens if you cache, then someone deletes the path from outside apache? - /* IMP1 Surely it would be more efficient to check the folders for - * the audit log repository base path in the configuration phase, to reduce - * the work we do on every request. Also, since our path depends on time, - * we could cache the time we last checked and don't check if we know - * the folder is there. - */ - rc = apr_dir_make_recursive(entry_basename, CREATEMODE_DIR, msr->mp); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 14:24:02:378 GMT-08:00 - 2008-01-07 :: 14:50:55:762 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - We already have support for relative filenames, but cannot get to this data from here. This needs solved by passing more data to the validate function (cmd_parms rec). Maybe need a warning here stating we do not support them yet, or it might be confusing to users that we do not here but do elsewhere. - /* TODO Support relative filenames. */ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 14:33:59:432 GMT-08:00 - 2008-01-07 :: 15:59:35:056 GMT-08:00 - - brian - brian - apache2/re.h - item.type.label.suggestion - item.severity.label.trivial - Why not stored in op_param_data like @rx, etc. The param_data is used w/exec action for lua. - /* Compiled Lua script. */ - msc_script *script; - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 15:55:08:220 GMT-08:00 - 2008-01-07 :: 16:02:36:938 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - This assumes lua is the only type (which it is now), but should be re-writen with a script_rec stored in param_data. - if (action->param_data != NULL) { /* Lua */ - msc_script *script = (msc_script *)action->param_data; - char *my_error_msg = NULL; - - if (lua_execute(script, NULL, msr, rule, &my_error_msg) < 0) { - msr_log(msr, 1, "%s", my_error_msg); - return 0; - } - } else { /* Execute as shell script. */ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 16:00:51:901 GMT-08:00 - 2008-01-07 :: 16:04:13:185 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - Not sure using an extension is a good idea here. Better I think would be to specify a type: "exec:[type=]/path/to/file" as in "exec:lua=/path/to/script" and make param_data a script_rec with a type and value. Also we use the abstract param_data here vs using a specific field as in SecRuleScript. - /* Process Lua scripts internally. */ - if (strlen(filename) > 4) { - char *p = filename + strlen(filename) - 4; - if ((p[0] == '.')&&(p[1] == 'l')&&(p[2] == 'u')&&(p[3] == 'a')) { - /* It's a Lua script. */ - msc_script *script = NULL; - - /* Compile script. */ - char *msg = lua_compile(&script, filename, engine->mp); - if (msg != NULL) return msg; - - action->param_data = script; - } - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 16:08:53:365 GMT-08:00 - 2008-01-11 :: 15:25:58:269 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.suggestion - item.severity.label.trivial - Should not log_escape the actions as they will get double escaped (once now and again when logged). - } else { - rule->unparsed = apr_psprintf(ruleset->mp, "SecRuleScript \"%s\" \"%s\"", - script_filename, log_escape(ruleset->mp, actions)); - } - - - item.resolution.label.invalidWontfix - item.status.label.closed - - - - 2008-01-07 :: 16:17:58:895 GMT-08:00 - 2008-01-11 :: 15:26:18:631 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.suggestion - item.severity.label.trivial - Should not log_escape the actions as they will get double escaped (once now and again when logged). - /* Add the unparsed rule */ - if ((strcmp(SECACTION_TARGETS, targets) == 0) && (strcmp(SECACTION_ARGS, args) == 0)) { - rule->unparsed = apr_psprintf(ruleset->mp, "SecAction \"%s\"", - log_escape(ruleset->mp, actions)); - } - else - if ((strcmp(SECMARKER_TARGETS, targets) == 0) - && (strcmp(SECMARKER_ARGS, args) == 0) - && (strncmp(SECMARKER_BASE_ACTIONS, actions, strlen(SECMARKER_BASE_ACTIONS)) == 0)) - { - rule->unparsed = apr_psprintf(ruleset->mp, "SecMarker \"%s\"", - log_escape(ruleset->mp, actions + strlen(SECMARKER_BASE_ACTIONS))); - } - else { - if (actions == NULL) { - rule->unparsed = apr_psprintf(ruleset->mp, "SecRule \"%s\" \"%s\"", - log_escape(ruleset->mp, targets), log_escape(ruleset->mp, args)); - } else { - rule->unparsed = apr_psprintf(ruleset->mp, "SecRule \"%s\" \"%s\" \"%s\"", - log_escape(ruleset->mp, targets), log_escape(ruleset->mp, args), - log_escape(ruleset->mp, actions)); - } - } - - - item.resolution.label.invalidWontfix - item.status.label.closed - - - - 2008-01-07 :: 16:22:43:295 GMT-08:00 - 2008-01-11 :: 15:26:48:665 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.suggestion - item.severity.label.trivial - No logging should be done here as we are passing the error_msg back to the parent and they are responsible for this. - if (*error_msg != NULL) { - /* ENH Shouldn't we log the problem? */ - return NULL; - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 16:30:41:403 GMT-08:00 - 2008-01-07 :: 16:31:00:930 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.missing - item.severity.label.trivial - Need to log on failure. - var = msre_create_var(ruleset, telts[i].key, telts[i].val, NULL, error_msg); - if (var == NULL) return -1; - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 16:36:31:466 GMT-08:00 - 2008-01-07 :: 16:36:54:189 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.suggestion - item.severity.label.trivial - Should replace with isvarnamechar() if possible. - while((*p != '\0')&&(*p != '|')&&(*p != ':')&&(*p != ',')&&(!isspace(*p))) p++; /* ENH replace with isvarnamechar() */ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 16:39:21:316 GMT-08:00 - 2008-01-11 :: 15:28:02:997 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.suggestion - item.severity.label.trivial - Fix or remove TODO. - // TODO better 64-bit support here - *error_msg = apr_psprintf(mp, "Missing closing quote at position %d: %s", - (int)(p - text), text); - free(value); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 16:39:47:318 GMT-08:00 - 2008-01-11 :: 15:28:24:355 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.suggestion - item.severity.label.trivial - Fix or remove TODO. - // TODO better 64-bit support here - *error_msg = apr_psprintf(mp, "Invalid quoted pair at position %d: %s", - (int)(p - text), text); - free(value); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 16:40:15:634 GMT-08:00 - 2008-01-11 :: 16:07:58:168 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.clarity - item.severity.label.trivial - Add parens for clarity. - *d++ = *p++; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 16:40:43:464 GMT-08:00 - 2008-01-11 :: 16:07:34:306 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.clarity - item.severity.label.trivial - Add parens for clarity. - *d++ = *p++; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 20:46:50:832 GMT-08:00 - 2008-01-11 :: 16:06:54:324 GMT-08:00 - - brian - brian - apache2/acmp.c - item.type.label.clarity - item.severity.label.trivial - Add parens for clarity. - *ucs_chars++ = *c++; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 20:47:54:093 GMT-08:00 - 2008-01-11 :: 16:06:02:231 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.clarity - item.severity.label.trivial - Add parens for clarity. - *t++ = *p++; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 20:48:42:400 GMT-08:00 - 2008-01-11 :: 16:05:23:686 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.clarity - item.severity.label.trivial - Add parens for clarity. - *d++ = *s++; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 21:42:51:192 GMT-08:00 - 2008-01-07 :: 21:44:12:097 GMT-08:00 - - brian - brian - apache2/apache2_io.c - item.type.label.programLogic - item.severity.label.major - Returning here may fail to free chunks data due to modsecurity_request_body_end() not being called. - int rcbs = modsecurity_request_body_store(msr, buf, buflen, error_msg); - if (rcbs < 0) { - if (rcbs == -5) { - *error_msg = apr_psprintf(msr->mp, "Requests body no files data length is larger than the " - "configured limit (%lu).", msr->txcfg->reqbody_no_files_limit); - return -5; - } - - return -1; - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 21:50:32:890 GMT-08:00 - 2008-01-07 :: 21:52:29:239 GMT-08:00 - - brian - brian - apache2/modsecurity.c - item.type.label.suggestion - item.severity.label.major - Good. This looks to solve the other issues noted as possible memory leaks in body chunk data due to modsecurity_request_body_end() not being called. Need to verify, though. - /* Register TX cleanup */ - apr_pool_cleanup_register(msr->mp, msr, modsecurity_tx_cleanup, apr_pool_cleanup_null); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 21:59:33:579 GMT-08:00 - 2008-01-11 :: 15:29:21:240 GMT-08:00 - - brian - brian - apache2/msc_util.c - item.type.label.suggestion - item.severity.label.trivial - Actually, the parens are *required* for correctness, so remove the comments. - (*invalid_count)++; /* parens quiet compiler warning */ - } - } else { - /* Not enough bytes available, copy the raw bytes. */ - *d++ = input[i++]; - count ++; - (*invalid_count)++; /* parens quiet compiler warning */ - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 22:03:31:138 GMT-08:00 - 2008-01-07 :: 22:04:07:108 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.irrelevant - item.severity.label.trivial - Does not appear to be used anywhere. - /** - * Destroys an engine instance, releasing the consumed memory. - */ -void msre_engine_destroy(msre_engine *engine) { - /* Destroyed automatically by the parent pool. - * apr_pool_destroy(engine->mp); - */ -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 22:04:22:398 GMT-08:00 - 2008-01-07 :: 22:04:35:724 GMT-08:00 - - brian - brian - apache2/re.h - item.type.label.irrelevant - item.severity.label.trivial - Does not appear to be used anywhere. - void DSOLOCAL msre_engine_destroy(msre_engine *engine); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 22:23:48:909 GMT-08:00 - 2008-01-11 :: 16:04:30:061 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.clarity - item.severity.label.trivial - This version should be moved up next to the normal version. - #if defined(PERFORMANCE_MEASUREMENT) -apr_status_t msre_ruleset_process_phase(msre_ruleset *ruleset, modsec_rec *msr) { - ... -} - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 22:30:42:215 GMT-08:00 - 2008-01-11 :: 16:02:09:916 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.missing - item.severity.label.major - Hmm, I thought this had already been fixed in trunk. Missing logging phase. Need to fix in 2.1.5 as well. - /** - * Removes from the ruleset all rules that match the given exception. - */ -int msre_ruleset_rule_remove_with_exception(msre_ruleset *ruleset, rule_exception *re) { - int count = 0; - - if (ruleset == NULL) return 0; - - count += msre_ruleset_phase_rule_remove_with_exception(ruleset, re, ruleset->phase_request_headers); - count += msre_ruleset_phase_rule_remove_with_exception(ruleset, re, ruleset->phase_request_body); - count += msre_ruleset_phase_rule_remove_with_exception(ruleset, re, ruleset->phase_response_headers); - count += msre_ruleset_phase_rule_remove_with_exception(ruleset, re, ruleset->phase_response_body); - - return count; -} - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 22:46:51:423 GMT-08:00 - 2008-01-11 :: 16:01:31:111 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.optimization - item.severity.label.trivial - Should move this to a static global for performance. - static const char *const severities[] = { - "EMERGENCY", - "ALERT", - "CRITICAL", - "ERROR", - "WARNING", - "NOTICE", - "INFO", - "DEBUG", - NULL, - }; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 22:49:04:822 GMT-08:00 - 2008-01-07 :: 22:49:16:740 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.suggestion - item.severity.label.trivial - Implement TODO. - //TODO: restrict to 512 bytes - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 22:51:40:727 GMT-08:00 - 2008-01-07 :: 22:53:13:118 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.optimization - item.severity.label.trivial - tags set to NULL would be a bit better as it would stop apr_pstrcat() earlier, but tags *must* remain last or wierd results. - char *tags = ""; - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-08 :: 11:44:13:484 GMT-08:00 - 2008-01-08 :: 11:45:29:864 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.optimization - item.severity.label.trivial - This causes two loops through the action list. Perhaps there is a more performant way to do these at the same time? Maybe split into two lists? - /* Perform non-disruptive actions. */ - msre_perform_nondisruptive_actions(msr, rule, rule->actionset, mptmp); - - /* Perform disruptive actions, but only if - * this rule is not part of a chain. - */ - if (rule->actionset->is_chained == 0) { - msre_perform_disruptive_actions(msr, rule, acting_actionset, mptmp, my_error_msg); - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-08 :: 12:16:20:280 GMT-08:00 - 2008-01-11 :: 15:59:48:300 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.irrelevant - item.severity.label.trivial - These do not appear to be needed. - tfnspath = NULL; - tfnskey = NULL; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-08 :: 12:20:29:491 GMT-08:00 - 2008-01-11 :: 15:59:23:210 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.programLogic - item.severity.label.major - This does not appear to work as the tfnskey is not being built here. Need to build the tfnskey in this loop for this to work. - /* check cache, saving the 'most complete' */ - crec = (msre_cache_rec *)apr_table_get(cachetab, tfnskey); - if (crec != NULL) { - last_crec = crec; - last_cached_tfn = tfnscount; - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-08 :: 21:29:14:889 GMT-08:00 - 2008-01-08 :: 21:30:41:759 GMT-08:00 - - brian - brian - apache2/re_tfns.c - item.type.label.optimization - item.severity.label.trivial - No need to set this on all. Only set it once when we find the first non-space char. - (*rval)[i] = '\0'; - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-08 :: 22:09:10:463 GMT-08:00 - 2008-01-11 :: 14:20:53:411 GMT-08:00 - - brian - brian - apache2/re_variables.c - item.type.label.suggestion - item.severity.label.trivial - Indention off. - return var_simple_generate_ex(var, vartab, mptmp, - apr_pmemdup(mptmp, - msr->matched_var->value, - msr->matched_var->value_len), - msr->matched_var->value_len); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-08 :: 22:09:35:664 GMT-08:00 - 2008-01-11 :: 14:21:28:242 GMT-08:00 - - brian - brian - apache2/re_variables.c - item.type.label.suggestion - item.severity.label.trivial - Indention off. - return var_simple_generate_ex(var, vartab, mptmp, - apr_pmemdup(mptmp, - msr->matched_var->name, - msr->matched_var->name_len), - msr->matched_var->name_len); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 10:55:38:639 GMT-08:00 - 2008-01-11 :: 14:25:15:860 GMT-08:00 - - brian - brian - apache2/acmp.c - item.type.label.suggestion - item.severity.label.trivial - Remove comment. - //return acmp_child_for_code(node, letter) - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 11:01:01:815 GMT-08:00 - 2008-01-11 :: 15:55:50:363 GMT-08:00 - - brian - brian - apache2/acmp.c - item.type.label.suggestion - item.severity.label.trivial - Change to #idef DEBUG_ACMP or similar. - /* printf("%c ->left %c \n", node->node->letter, node->left->node->letter); */ - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 11:01:41:424 GMT-08:00 - 2008-01-11 :: 15:56:10:175 GMT-08:00 - - brian - brian - apache2/acmp.c - item.type.label.suggestion - item.severity.label.trivial - Change to #idef DEBUG_ACMP or similar. - /* printf("%c ->right %c \n", node->node->letter, node->right->node->letter); */ - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 11:02:22:279 GMT-08:00 - 2008-01-11 :: 15:56:41:045 GMT-08:00 - - brian - brian - apache2/acmp.c - item.type.label.suggestion - item.severity.label.trivial - Change to #idef DEBUG_ACMP or similar. - /* printf("fail direction: *%s* => *%s*\n", child->text, child->fail->text); */ - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 11:02:42:897 GMT-08:00 - 2008-01-11 :: 15:54:16:807 GMT-08:00 - - brian - brian - apache2/acmp.c - item.type.label.suggestion - item.severity.label.trivial - Change to #idef DEBUG_ACMP or similar. - /* printf("fail direction: *%s* => *%s*\n", node->text, node->fail->text); */ - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 11:10:25:157 GMT-08:00 - 2008-01-11 :: 14:24:57:212 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - The 'tag' and 'severity' metadata actions should be included. Some actions are missing from the log msg. - /* Must NOT use metadata actions. */ - if ((rule->actionset->id != NOT_SET_P) - ||(rule->actionset->rev != NOT_SET_P) - ||(rule->actionset->msg != NOT_SET_P) - ||(rule->actionset->logdata != NOT_SET_P)) - { - return apr_psprintf(cmd->pool, "ModSecurity: Metadata actions (id, rev, msg) " - " can only be specified by chain starter rules."); - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 11:18:38:672 GMT-08:00 - 2008-01-11 :: 14:29:49:134 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Probably should check we were able to allocate. - msre_rule *phrule = apr_palloc(rule->ruleset->mp, sizeof(msre_rule)); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:05:40:949 GMT-08:00 - 2008-01-11 :: 14:30:01:771 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Comment or remove. - TODO - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:06:48:653 GMT-08:00 - 2008-01-11 :: 14:30:40:514 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - ENH instead of TODO - TODO - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:09:17:114 GMT-08:00 - 2008-01-11 :: 14:31:50:812 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - The 'tag' and 'severity' metadata actions should be included. Some actions are missing from the log msg. - /* Must not use metadata actions. */ - if ((dcfg->tmp_default_actionset->id != NOT_SET_P) - ||(dcfg->tmp_default_actionset->rev != NOT_SET_P) - ||(dcfg->tmp_default_actionset->msg != NOT_SET_P) - ||(dcfg->tmp_default_actionset->logdata != NOT_SET_P)) - { - return apr_psprintf(cmd->pool, "ModSecurity: SecDefaultAction must not " - "contain any metadata actions (id, rev, msg)."); - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:17:06:712 GMT-08:00 - 2008-01-11 :: 14:33:23:560 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or make ENH - // TODO Validate encoding - // return apr_psprintf(cmd->pool, "ModSecurity: Invalid value for SecRequestBodyAccess: %s", p1); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:17:55:545 GMT-08:00 - 2008-01-09 :: 12:25:33:617 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.irrelevant - item.severity.label.trivial - Remove code? - /* -static const char *cmd_rule_import_by_id(cmd_parms *cmd, void *_dcfg, const char *p1) { - directory_config *dcfg = (directory_config *)_dcfg; - rule_exception *re = apr_pcalloc(cmd->pool, sizeof(rule_exception)); - if (dcfg == NULL) return NULL; - - re->type = RULE_EXCEPTION_IMPORT_ID; - // TODO verify p1 - re->param = p1; - *(rule_exception **)apr_array_push(dcfg->rule_exceptions) = re; - - return NULL; -} - -static const char *cmd_rule_import_by_msg(cmd_parms *cmd, void *_dcfg, const char *p1) { - directory_config *dcfg = (directory_config *)_dcfg; - rule_exception *re = apr_pcalloc(cmd->pool, sizeof(rule_exception)); - if (dcfg == NULL) return NULL; - - re->type = RULE_EXCEPTION_IMPORT_MSG; - // TODO verify p1 - re->param = p1; - *(rule_exception **)apr_array_push(dcfg->rule_exceptions) = re; - - return NULL; -} -*/ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 12:18:34:399 GMT-08:00 - 2008-01-11 :: 14:34:07:673 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix or TODO->ENH - // TODO enforce format (letters, digits, ., _, -) - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:20:16:234 GMT-08:00 - 2008-01-11 :: 14:40:53:633 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Need to allow for relative filename based of rule file. - /* -- Geo Lookup configuration -- */ - -static const char *cmd_geo_lookup_db(cmd_parms *cmd, void *_dcfg, - const char *p1) -{ - char *error_msg; - directory_config *dcfg = (directory_config *)_dcfg; - if (dcfg == NULL) return NULL; - - if (geo_init(dcfg, p1, &error_msg) <= 0) { - return error_msg; - } - - return NULL; -} - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:22:58:798 GMT-08:00 - 2008-01-09 :: 12:23:11:462 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Portable? - /* The NOT_SET indicator is -1, a signed long, and therfore - * we cannot be >= the unsigned value of NOT_SET. - */ - if ((unsigned long)intval >= (unsigned long)NOT_SET) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations minlen must be less than: %lu", (unsigned long)NOT_SET); - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 12:23:33:676 GMT-08:00 - 2008-01-09 :: 12:23:39:437 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Portable? - /* The NOT_SET indicator is -1, a signed long, and therfore - * we cannot be >= the unsigned value of NOT_SET. - */ - if ((unsigned long)intval >= (unsigned long)NOT_SET) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations maxlen must be less than: %lu", (unsigned long)NOT_SET); - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 12:24:06:001 GMT-08:00 - 2008-01-09 :: 12:24:23:994 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Could use strtol as Ivan has as well. - intval = apr_atoi64(charval); - if (errno == ERANGE) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations minlen out of range: %s", charval); - } - if (intval < 0) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations minlen must be positive: %s", charval); - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 12:24:37:453 GMT-08:00 - 2008-01-09 :: 12:24:42:462 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Could use strtol as Ivan has as well. - intval = apr_atoi64(charval); - if (errno == ERANGE) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations maxlen out of range: %s", charval); - } - if (intval < 0) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations maxlen must be positive: %s", charval); - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 12:25:36:799 GMT-08:00 - 2008-01-09 :: 12:31:35:264 GMT-08:00 - - brian - brian - apache2/apache2_io.c - item.type.label.programLogic - item.severity.label.major - Remove code? It is actually used below, so need to verify. - #if 0 -static void dummy_free_func(void *data) {} -#endif - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 12:27:10:252 GMT-08:00 - 2008-01-09 :: 12:29:31:617 GMT-08:00 - - brian - brian - apache2/apache2_io.c - item.type.label.programLogic - item.severity.label.major - dummy_free_func() is defined where? It is ifdef'd out at the top of source, so need to verify it is valid. - /* Do not make a copy of the data we received in the chunk. */ - bucket = apr_bucket_heap_create(chunk->data, chunk->length, dummy_free_func, - f->r->connection->bucket_alloc); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 12:35:21:613 GMT-08:00 - 2008-01-09 :: 13:02:40:668 GMT-08:00 - - brian - brian - apache2/apache2_io.c - item.type.label.suggestion - item.severity.label.trivial - Yes, why do we ignore the rc - why have one at all? - // TODO: Why ignore the return code here? - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 12:55:19:633 GMT-08:00 - 2008-01-11 :: 17:00:04:416 GMT-08:00 - - brian - brian - apache2/msc_reqbody.c - item.type.label.suggestion - item.severity.label.trivial - Portable way to format sizeof()? - *error_msg = apr_psprintf(msr->mp, "Input filter: Failed to allocate %lu bytes for request body chunk.", (unsigned long)sizeof(msc_data_chunk)); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:55:39:585 GMT-08:00 - 2008-01-11 :: 16:58:16:438 GMT-08:00 - - brian - brian - apache2/msc_reqbody.c - item.type.label.suggestion - item.severity.label.trivial - Portable way to format sizeof()? - *error_msg = apr_psprintf(msr->mp, "Failed to allocate %lu bytes for request body disk chunk.", (unsigned long)sizeof(msc_data_chunk)); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:56:05:302 GMT-08:00 - 2008-01-11 :: 16:57:58:049 GMT-08:00 - - brian - brian - apache2/msc_reqbody.c - item.type.label.suggestion - item.severity.label.trivial - Portable way to format sizeof()? - *error_msg = apr_psprintf(msr->mp, "Failed to allocate %lu bytes for request body disk chunk.", (unsigned long)sizeof(msc_data_chunk)); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:56:51:734 GMT-08:00 - 2008-01-11 :: 14:41:50:773 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - // TODO check whether the parameter is a valid MIME type of "null" - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:58:11:331 GMT-08:00 - 2008-01-11 :: 14:42:59:645 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - AP_INIT_TAKE1 ( - "SecAction", - cmd_action, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:58:29:461 GMT-08:00 - 2008-01-11 :: 14:46:15:813 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - AP_INIT_TAKE1 ( - "SecDataDir", - cmd_data_dir, - NULL, - CMD_SCOPE_MAIN, - "" // TODO - ), - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:58:45:647 GMT-08:00 - 2008-01-11 :: 14:46:51:173 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - AP_INIT_TAKE1 ( - "SecDefaultAction", - cmd_default_action, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:59:03:959 GMT-08:00 - 2008-01-11 :: 14:47:05:294 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - AP_INIT_TAKE1 ( - "SecResponseBodyLimit", - cmd_response_body_limit, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:59:17:580 GMT-08:00 - 2008-01-11 :: 14:49:00:349 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - AP_INIT_TAKE1 ( - "SecResponseBodyLimitAction", - cmd_response_body_limit_action, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:00:04:205 GMT-08:00 - 2008-01-11 :: 14:50:22:367 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - AP_INIT_TAKE23 ( - "SecRule", - cmd_rule, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:00:30:606 GMT-08:00 - 2008-01-11 :: 14:51:16:772 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - AP_INIT_TAKE12 ( - "SecRuleScript", - cmd_rule_script, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - AP_INIT_ITERATE ( - "SecRuleRemoveById", - cmd_rule_remove_by_id, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - AP_INIT_ITERATE ( - "SecRuleRemoveByMsg", - cmd_rule_remove_by_msg, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:00:56:319 GMT-08:00 - 2008-01-11 :: 14:52:48:192 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - AP_INIT_TAKE1 ( - "SecTmpDir", - cmd_tmp_dir, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - AP_INIT_TAKE1 ( - "SecUploadDir", - cmd_upload_dir, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - AP_INIT_TAKE1 ( - "SecUploadKeepFiles", - cmd_upload_keep_files, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - AP_INIT_TAKE1 ( - "SecWebAppId", - cmd_web_app_id, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:01:32:996 GMT-08:00 - 2008-01-11 :: 14:54:32:209 GMT-08:00 - - brian - brian - apache2/mod_security2.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - /* Update the request headers. They might have changed after - * the body was read (trailers). - */ - // TODO We still need to keep a copy of the original headers - // to log in the audit log. - msr->request_headers = apr_table_copy(msr->mp, r->headers_in); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:02:05:035 GMT-08:00 - 2008-01-09 :: 13:02:47:482 GMT-08:00 - - brian - brian - apache2/modsecurity.c - item.type.label.suggestion - item.severity.label.trivial - Yes, why do we ignore the rc - why have one at all? - // TODO: Why do we ignore return code here? - modsecurity_request_body_clear(msr, &my_error_msg); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:03:24:928 GMT-08:00 - 2008-01-09 :: 13:04:49:633 GMT-08:00 - - brian - brian - apache2/msc_geo.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO. - offset = -3; - apr_file_seek(geo->db, APR_END, &offset); - /* TODO check offset */ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:04:20:937 GMT-08:00 - 2008-01-09 :: 13:04:39:295 GMT-08:00 - - brian - brian - apache2/msc_geo.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO. - rc = apr_file_read_full(geo->db, &buf, 1, &nbytes); - /* TODO: check rc */ - geo->dbtype = (int)buf[0]; - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:05:16:204 GMT-08:00 - 2008-01-09 :: 13:05:27:720 GMT-08:00 - - brian - brian - apache2/msc_geo.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO. - apr_file_seek(geo->db, APR_SET, &seekto); - /* TODO: check rc */ - rc = apr_file_read_full(geo->db, &buf, (2 * reclen), &nbytes); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:05:58:953 GMT-08:00 - 2008-01-09 :: 13:06:03:494 GMT-08:00 - - brian - brian - apache2/msc_geo.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO. - apr_file_seek(geo->db, APR_SET, &seekto); - /* TODO: check rc */ - rc = apr_file_read_full(geo->db, &cbuf, sizeof(cbuf), &nbytes); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:06:45:422 GMT-08:00 - 2008-01-11 :: 14:55:10:355 GMT-08:00 - - brian - brian - apache2/msc_logging.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - /* The audit log storage directory should be explicitly - * defined. But if it isn't try to write to the same - * directory where the index file is placed. Of course, - * it is *very* bad practice to allow the Apache user - * to write to the same directory where a root user is - * writing to but it's not us that's causing the problem - * and there isn't anything we can do about that. - * - * TODO Actually there is something we can do! We will make - * SecAuditStorageDir mandatory, ask the user to explicitly - * define the storage location *and* refuse to work if the - * index and the storage location are in the same folder. - */ - if (msr->txcfg->auditlog_storage_dir == NULL) { - entry_filename = file_dirname(msr->mp, msr->txcfg->auditlog_name); - } - else { - entry_filename = msr->txcfg->auditlog_storage_dir; - } - if (entry_filename == NULL) return; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:07:22:398 GMT-08:00 - 2008-01-11 :: 14:55:42:896 GMT-08:00 - - brian - brian - apache2/msc_logging.c - item.type.label.suggestion - item.severity.label.trivial - Change TODO to ENH. - /* AUDITLOG_PART_UPLOADS */ - // TODO: Implement - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:08:02:613 GMT-08:00 - 2008-01-11 :: 15:51:28:546 GMT-08:00 - - brian - brian - apache2/msc_lua.c - item.type.label.missing - item.severity.label.trivial - Log an error. - } else { - // TODO Error - return NULL; - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:08:51:978 GMT-08:00 - 2008-01-09 :: 13:09:44:680 GMT-08:00 - - brian - brian - apache2/msc_util.c - item.type.label.suggestion - item.severity.label.normal - Fix TODO. - char *resolve_relative_path(apr_pool_t *pool, const char *parent_filename, const char *filename) { - if (filename == NULL) return NULL; - // TODO Support paths on operating systems other than Unix. - if (filename[0] == '/') return (char *)filename; - - return apr_pstrcat(pool, apr_pstrndup(pool, parent_filename, - strlen(parent_filename) - strlen(apr_filepath_name_get(parent_filename))), - filename, NULL); -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:10:22:786 GMT-08:00 - 2008-01-09 :: 13:10:31:982 GMT-08:00 - - brian - brian - apache2/pdf_protect.c - item.type.label.suggestion - item.severity.label.trivial - Change from TODO to ENH. - // TODO We need ID and REV values for the PDF XSS alert. - -// TODO It would be nice if the user could choose the ID/REV/SEVERITY/MESSAGE, etc. - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:10:53:181 GMT-08:00 - 2008-01-09 :: 13:11:06:931 GMT-08:00 - - brian - brian - apache2/pdf_protect.c - item.type.label.suggestion - item.severity.label.trivial - Change from TODO to ENH. - // TODO Should we look at err_headers_out too? - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:11:23:251 GMT-08:00 - 2008-01-09 :: 13:11:28:132 GMT-08:00 - - brian - brian - apache2/pdf_protect.c - item.type.label.suggestion - item.severity.label.trivial - Change from TODO to ENH. - // TODO application/x-pdf, application/vnd.fdf, application/vnd.adobe.xfdf, - // application/vnd.adobe.xdp+xml, application/vnd.adobe.xfd+xml, application/vnd.pdf - // application/acrobat, text/pdf, text/x-pdf ??? - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:11:54:942 GMT-08:00 - 2008-01-09 :: 13:12:24:617 GMT-08:00 - - brian - brian - apache2/pdf_protect.c - item.type.label.missing - item.severity.label.trivial - Add the missing alert. - // TODO Log alert - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:13:04:159 GMT-08:00 - 2008-01-09 :: 13:13:38:064 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - Not positive why the TODO here. Perhaps for a decision as to log and/or at what level? - msr_log(msr, 4, "Ctl: Set auditEngine to %d.", msr->txcfg->auditlog_flag); // TODO - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:14:02:379 GMT-08:00 - 2008-01-11 :: 15:42:21:749 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.suggestion - item.severity.label.trivial - Change from TODO to ENH. - // TODO Write & use string_ends(s, e). - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:14:29:571 GMT-08:00 - 2008-01-11 :: 15:43:53:997 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.suggestion - item.severity.label.trivial - Remove the ifdef as lua is required? - #ifdef WITH_LUA - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:17:47:759 GMT-08:00 - 2008-01-11 :: 15:41:12:538 GMT-08:00 - - brian - brian - CHANGES - item.type.label.suggestion - item.severity.label.trivial - Remove TODO. - TODO: more to come - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:35:00:891 GMT-08:00 - 2008-01-09 :: 13:35:40:975 GMT-08:00 - - brian - brian - apache2/modsecurity.h - item.type.label.optimization - item.severity.label.trivial - Need to re-test implementing this as just a table. - /* data cache */ - apr_hash_t *tcache; - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:37:04:264 GMT-08:00 - 2008-01-09 :: 13:37:25:196 GMT-08:00 - - brian - brian - apache2/modsecurity.h - item.type.label.suggestion - item.severity.label.trivial - Should probably use STRINGIFY and define the numeric value. - #define MODSEC_VERSION_MAJOR "2" -#define MODSEC_VERSION_MINOR "5" -#define MODSEC_VERSION_MAINT "0" -#define MODSEC_VERSION_TYPE "rc" -#define MODSEC_VERSION_RELEASE "1" - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 14:08:14:025 GMT-08:00 - 2008-01-11 :: 15:40:12:054 GMT-08:00 - - brian - brian - apache2/msc_logging.c - item.type.label.suggestion - item.severity.label.trivial - Spelling. - throught - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 14:21:32:782 GMT-08:00 - 2008-01-09 :: 14:22:32:803 GMT-08:00 - - brian - brian - apache2/msc_geo.h - item.type.label.suggestion - item.severity.label.trivial - This value may not be portable based on endianness. The algorithm compares it to the IP address as int in host order. - #define GEO_COUNTRY_OFFSET 0xffff00 - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 14:31:25:720 GMT-08:00 - 2008-01-09 :: 14:45:20:612 GMT-08:00 - - brian - brian - apache2/msc_geo.c - item.type.label.suggestion - item.severity.label.trivial - Check portability due to endianness. It seems that the DB values are assumed to be in host order. Perhaps the compiled DB is little-endian and we need to compensate? - - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 15:28:14:563 GMT-08:00 - 2008-01-09 :: 15:28:24:203 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.irrelevant - item.severity.label.trivial - Remove code? - #if 0 -static char *multipart_construct_filename(modsec_rec *msr) { - char c, *p, *q = msr->mpd->mpp->filename; - char *filename; - - /* find the last backward slash and consider the - * filename to be only what's right from it - */ - p = strrchr(q, '\\'); - if (p != NULL) q = p + 1; - - /* do the same for the forward slash */ - p = strrchr(q, '/'); - if (p != NULL) q = p + 1; - - /* allow letters, digits and dots, replace - * everything else with underscores - */ - p = filename = apr_pstrdup(msr->mp, q); - while((c = *p) != 0) { - if (!( isalnum(c)||(c == '.') )) *p = '_'; - p++; - } - - return filename; -} -#endif - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 15:35:50:978 GMT-08:00 - 2008-01-09 :: 15:49:54:128 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.programLogic - item.severity.label.trivial - The multipart C-D header is case insensitive (rfc 2183), so we should probably use strncasecmp() here. - /* accept only what we understand */ - if (strncmp(c_d_value, "form-data", 9) != 0) { - return -1; - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 16:05:05:601 GMT-08:00 - 2008-01-09 :: 16:07:17:436 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.suggestion - item.severity.label.trivial - This allows for a name of "", so maybe check for name[0] == '\0' and return an error. Although we catch this later on as an unrecognized name and return -10. - start = p; - while((*p != '\0')&&(*p != '=')&&(*p != '\t')&&(*p != ' ')) p++; - if (*p == '\0') return -4; - - name = apr_pstrmemdup(msr->mp, start, (p - start)); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 16:14:36:307 GMT-08:00 - 2008-01-09 :: 17:02:47:324 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.suggestion - item.severity.label.trivial - For a quoted value we just include everything until the end quote. The field values should be US-ASCII 'qtext' or 'quoted-pair' (RFC 822) or escaped using RFC 2047. - if (*p == '"') { - *t = '\0'; - break; - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 16:59:12:520 GMT-08:00 - 2008-01-09 :: 17:00:55:566 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.suggestion - item.severity.label.trivial - I think this is wrong. RFC 822 defines a quoted-string as <"> *(qtext/quoted-pair) <"> and quoted-par being able to quote any CHAR. - /* only " and \ can be escaped */ - if ((*(p + 1) == '"')||(*(p + 1) == '\\')) { - p++; - } - else { - /* improper escaping */ - - /* We allow for now because IE sends - * improperly escaped content and there's - * nothing we can do about it. - * - * return -9; - */ - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 17:04:30:357 GMT-08:00 - 2008-01-09 :: 17:05:19:369 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.suggestion - item.severity.label.trivial - RFC 2045 defines an 'attribute' as "ALWAYS case-insensitive", so these should be strncasecmp() - if (strcmp(name, "name") == 0) { - if (msr->mpd->mpp->name != NULL) return -14; - msr->mpd->mpp->name = value; - - if (msr->txcfg->debuglog_level >= 9) { - msr_log(msr, 9, "Multipart: Content-Disposition name: %s", - log_escape_nq(msr->mp, value)); - } - } - else - if (strcmp(name, "filename") == 0) { - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-10 :: 12:24:59:317 GMT-08:00 - 2008-01-11 :: 16:58:44:275 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.suggestion - item.severity.label.trivial - len is always assumed to be at least 1. What is preventing a len of 0? - if (len > 1) { - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-10 :: 12:27:34:472 GMT-08:00 - 2008-01-11 :: 15:39:17:001 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.suggestion - item.severity.label.trivial - Use MULTIPART_BUF_SIZE for the constant. - if (strlen(new_value) > 4096) { - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-10 :: 13:04:27:048 GMT-08:00 - 2008-01-10 :: 13:04:57:229 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.suggestion - item.severity.label.trivial - Cannot find anything that supports that this is or is not allowed. - /* Flag for whitespace after parameter name. */ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-10 :: 13:05:06:226 GMT-08:00 - 2008-01-10 :: 13:05:11:552 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.suggestion - item.severity.label.trivial - Cannot find anything that supports that this is or is not allowed. - /* Flag for whitespace before parameter value. */ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-10 :: 13:12:20:299 GMT-08:00 - 2008-01-11 :: 15:38:39:261 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.suggestion - item.severity.label.trivial - Use '\r' vs 0x0d as is done elsewhere. - if ((c == 0x0d)&&(msr->mpd->bufleft == 1)) { - /* we don't want to take 0x0d as the last byte in the buffer */ - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-10 :: 13:12:54:295 GMT-08:00 - 2008-01-11 :: 15:36:43:726 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.suggestion - item.severity.label.trivial - Use '\n' vs 0x0a as is done elsewhere. - if ((c == 0x0a)||(msr->mpd->bufleft == 0)||(process_buffer)) { - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-10 :: 13:19:16:539 GMT-08:00 - 2008-01-11 :: 15:35:53:104 GMT-08:00 - - brian - brian - apache2/msc_parsers.c - item.type.label.suggestion - item.severity.label.trivial - Headers are not used. I think they were added for iconv support, but Ivan removed it. - #include "iconv.h" -#include <errno.h> - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-10 :: 13:26:56:609 GMT-08:00 - 2008-01-10 :: 13:27:21:817 GMT-08:00 - - brian - brian - apache2/msc_util.c - item.type.label.suggestion - item.severity.label.trivial - Be more consistent in naming. - #define VALID_HEX(X) (((X >= '0')&&(X <= '9')) || ((X >= 'a')&&(X <= 'f')) || ((X >= 'A')&&(X <= 'F'))) -#define ISODIGIT(X) ((X >= '0')&&(X <= '7')) - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-10 :: 13:32:29:278 GMT-08:00 - 2008-01-11 :: 16:51:13:950 GMT-08:00 - - brian - brian - apache2/msc_util.c - item.type.label.optimization - item.severity.label.trivial - No need to do a strlen twice *and* loop through the string. Just loop and exit on non-space. - if (strlen(string) == 0) return 1; - - for(i = 0; i < strlen(string); i++) { - if (!isspace(string[i])) { - return 0; - } - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-10 :: 13:35:40:307 GMT-08:00 - 2008-01-10 :: 13:36:03:933 GMT-08:00 - - brian - brian - apache2/modsecurity.c - item.type.label.irrelevant - item.severity.label.trivial - Remove commented code? - /* Serial audit log mutext */ - rc = apr_global_mutex_create(&msce->auditlog_lock, NULL, APR_LOCK_DEFAULT, mp); - if (rc != APR_SUCCESS) { - //ap_log_error(APLOG_MARK, APLOG_ERR, rv, s, "mod_security: Could not create modsec_auditlog_lock"); - //return HTTP_INTERNAL_SERVER_ERROR; - return -1; - } - - #ifdef __SET_MUTEX_PERMS - rc = unixd_set_global_mutex_perms(msce->auditlog_lock); - if (rc != APR_SUCCESS) { - // ap_log_error(APLOG_MARK, APLOG_ERR, rc, s, "mod_security: Could not set permissions on modsec_auditlog_lock; check User and Group directives"); - // return HTTP_INTERNAL_SERVER_ERROR; - return -1; - } - #endif - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-10 :: 13:36:28:229 GMT-08:00 - 2008-01-10 :: 13:36:52:702 GMT-08:00 - - brian - brian - apache2/modsecurity.c - item.type.label.suggestion - item.severity.label.trivial - What *should* we do on error here? - if (rc != APR_SUCCESS) { - // ap_log_error(APLOG_MARK, APLOG_ERR, rs, s, "Failed to child-init auditlog mutex"); - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-10 :: 13:37:17:326 GMT-08:00 - 2008-01-10 :: 13:37:25:682 GMT-08:00 - - brian - brian - apache2/modsecurity.c - item.type.label.suggestion - item.severity.label.trivial - This does nothing. - /** - * Releases resources held by engine instance. - */ -void modsecurity_shutdown(msc_engine *msce) { - if (msce == NULL) return; -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-10 :: 13:39:01:265 GMT-08:00 - 2008-01-10 :: 13:39:22:070 GMT-08:00 - - brian - brian - apache2/modsecurity.c - item.type.label.suggestion - item.severity.label.trivial - Figure out the optimal initial size for the arrays/tables. - /* Collections. */ - msr->tx_vars = apr_table_make(msr->mp, 32); - if (msr->tx_vars == NULL) return -1; - - msr->geo_vars = apr_table_make(msr->mp, 8); - if (msr->geo_vars == NULL) return -1; - - msr->collections = apr_table_make(msr->mp, 8); - if (msr->collections == NULL) return -1; - msr->collections_dirty = apr_table_make(msr->mp, 8); - if (msr->collections_dirty == NULL) return -1; - - /* Other */ - msr->tcache = apr_hash_make(msr->mp); - if (msr->tcache == NULL) return -1; - - msr->matched_rules = apr_array_make(msr->mp, 16, sizeof(void *)); - if (msr->matched_rules == NULL) return -1; - - msr->matched_var = (msc_string *)apr_pcalloc(msr->mp, sizeof(msc_string)); - if (msr->matched_var == NULL) return -1; - - msr->highest_severity = 255; /* high, invalid value */ - - msr->removed_rules = apr_array_make(msr->mp, 16, sizeof(char *)); - if (msr->removed_rules == NULL) return -1; - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-10 :: 13:42:33:518 GMT-08:00 - 2008-01-10 :: 13:42:50:351 GMT-08:00 - - brian - brian - apache2/msc_xml.c - item.type.label.suggestion - item.severity.label.trivial - Remove #if 0'd code? - #if 0 -static void xml_receive_sax_error(void *data, const char *msg, ...) { - modsec_rec *msr = (modsec_rec *)data; - char message[256]; - - if (msr == NULL) return; - - apr_snprintf(message, sizeof(message), "%s (line %d offset %d)", - log_escape_nq(msr->mp, msr->xml->parsing_ctx->lastError.message), - msr->xml->parsing_ctx->lastError.line, - msr->xml->parsing_ctx->lastError.int2); - - msr_log(msr, 5, "XML: Parsing error: %s", message); -} -#endif - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-10 :: 13:54:16:193 GMT-08:00 - 2008-01-10 :: 13:54:45:610 GMT-08:00 - - brian - brian - apache2/mod_security2.c - item.type.label.suggestion - item.severity.label.trivial - What is the history of this? - /* Our own hook to handle RPC transactions (not used at the moment). - * // ap_hook_handler(hook_handler, NULL, NULL, APR_HOOK_MIDDLE); - */ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-10 :: 13:56:58:413 GMT-08:00 - 2008-01-11 :: 15:34:14:676 GMT-08:00 - - brian - brian - apache2/msc_lua.c - item.type.label.suggestion - item.severity.label.trivial - Change C++ to C style comment. - // Get the response from the script. - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - diff --git a/review/pre-2.5-brian.txt b/review/pre-2.5-brian.txt deleted file mode 100644 index 3f2f49f9..00000000 --- a/review/pre-2.5-brian.txt +++ /dev/null @@ -1,982 +0,0 @@ -File: apache2/apache2_config.c -=================================================================== -[brian] Irrelevant @ 1251 - -Remove code? - -/* -static const char *cmd_rule_import_by_id(cmd_parms *cmd, void *_dcfg, const char *p1) { - directory_config *dcfg = (directory_config *)_dcfg; - rule_exception *re = apr_pcalloc(cmd->pool, sizeof(rule_exception)); - if (dcfg == NULL) return NULL; - - re->type = RULE_EXCEPTION_IMPORT_ID; - // TODO verify p1 - re->param = p1; - *(rule_exception **)apr_array_push(dcfg->rule_exceptions) = re; - - return NULL; -} - -static const char *cmd_rule_import_by_msg(cmd_parms *cmd, void *_dcfg, const char *p1) { - directory_config *dcfg = (directory_config *)_dcfg; - rule_exception *re = apr_pcalloc(cmd->pool, sizeof(rule_exception)); - if (dcfg == NULL) return NULL; - - re->type = RULE_EXCEPTION_IMPORT_MSG; - // TODO verify p1 - re->param = p1; - *(rule_exception **)apr_array_push(dcfg->rule_exceptions) = re; - - return NULL; -} -*/ - - -[brian] Suggestion @ 1514 - -Could use strtol as Ivan has as well. - -intval = apr_atoi64(charval); - if (errno == ERANGE) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations minlen out of range: %s", charval); - } - if (intval < 0) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations minlen must be positive: %s", charval); - } - - -[brian] Suggestion @ 1522 - -Portable? - -/* The NOT_SET indicator is -1, a signed long, and therfore - * we cannot be >= the unsigned value of NOT_SET. - */ - if ((unsigned long)intval >= (unsigned long)NOT_SET) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations minlen must be less than: %lu", (unsigned long)NOT_SET); - } - - -[brian] Suggestion @ 1534 - -Could use strtol as Ivan has as well. - -intval = apr_atoi64(charval); - if (errno == ERANGE) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations maxlen out of range: %s", charval); - } - if (intval < 0) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations maxlen must be positive: %s", charval); - } - - -[brian] Suggestion @ 1542 - -Portable? - -/* The NOT_SET indicator is -1, a signed long, and therfore - * we cannot be >= the unsigned value of NOT_SET. - */ - if ((unsigned long)intval >= (unsigned long)NOT_SET) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations maxlen must be less than: %lu", (unsigned long)NOT_SET); - } - - - -File: apache2/apache2_io.c -=================================================================== -[brian] ProgramLogic @ 17 - -Remove code? It is actually used below, so need to verify. - -#if 0 -static void dummy_free_func(void *data) {} -#endif - - -[brian] ProgramLogic @ 92 - -dummy_free_func() is defined where? It is ifdef'd out at the top of source, so need to verify it is valid. - -/* Do not make a copy of the data we received in the chunk. */ - bucket = apr_bucket_heap_create(chunk->data, chunk->length, dummy_free_func, - f->r->connection->bucket_alloc); - - -[brian] ProgramLogic @ 224 - -Returning here may fail to free chunks data due to modsecurity_request_body_end() not being called. - -int rcbs = modsecurity_request_body_store(msr, buf, buflen, error_msg); - if (rcbs < 0) { - if (rcbs == -5) { - *error_msg = apr_psprintf(msr->mp, "Requests body no files data length is larger than the " - "configured limit (%lu).", msr->txcfg->reqbody_no_files_limit); - return -5; - } - - return -1; - } - - -[brian] Suggestion @ 246 - -Yes, why do we ignore the rc - why have one at all? - -// TODO: Why ignore the return code here? - - - -File: apache2/mod_security2.c -=================================================================== -[brian] Suggestion @ 1074 - -What is the history of this? - -/* Our own hook to handle RPC transactions (not used at the moment). - * // ap_hook_handler(hook_handler, NULL, NULL, APR_HOOK_MIDDLE); - */ - - - -File: apache2/modsecurity.c -=================================================================== -[brian] Irrelevant @ 100 - -Remove commented code? - -/* Serial audit log mutext */ - rc = apr_global_mutex_create(&msce->auditlog_lock, NULL, APR_LOCK_DEFAULT, mp); - if (rc != APR_SUCCESS) { - //ap_log_error(APLOG_MARK, APLOG_ERR, rv, s, "mod_security: Could not create modsec_auditlog_lock"); - //return HTTP_INTERNAL_SERVER_ERROR; - return -1; - } - - #ifdef __SET_MUTEX_PERMS - rc = unixd_set_global_mutex_perms(msce->auditlog_lock); - if (rc != APR_SUCCESS) { - // ap_log_error(APLOG_MARK, APLOG_ERR, rc, s, "mod_security: Could not set permissions on modsec_auditlog_lock; check User and Group directives"); - // return HTTP_INTERNAL_SERVER_ERROR; - return -1; - } - #endif - - -[brian] Suggestion @ 126 - -What *should* we do on error here? - -if (rc != APR_SUCCESS) { - // ap_log_error(APLOG_MARK, APLOG_ERR, rs, s, "Failed to child-init auditlog mutex"); - } - - -[brian] Suggestion @ 132 - -This does nothing. - -/** - * Releases resources held by engine instance. - */ -void modsecurity_shutdown(msc_engine *msce) { - if (msce == NULL) return; -} - - -[brian] Suggestion @ 178 - -Yes, why do we ignore the rc - why have one at all? - -// TODO: Why do we ignore return code here? - modsecurity_request_body_clear(msr, &my_error_msg); - - -[brian] Suggestion @ 196 - -Good. This looks to solve the other issues noted as possible memory leaks in body chunk data due to modsecurity_request_body_end() not being called. Need to verify, though. - -/* Register TX cleanup */ - apr_pool_cleanup_register(msr->mp, msr, modsecurity_tx_cleanup, apr_pool_cleanup_null); - - -[brian] Suggestion @ 298 - -Figure out the optimal initial size for the arrays/tables. - -/* Collections. */ - msr->tx_vars = apr_table_make(msr->mp, 32); - if (msr->tx_vars == NULL) return -1; - - msr->geo_vars = apr_table_make(msr->mp, 8); - if (msr->geo_vars == NULL) return -1; - - msr->collections = apr_table_make(msr->mp, 8); - if (msr->collections == NULL) return -1; - msr->collections_dirty = apr_table_make(msr->mp, 8); - if (msr->collections_dirty == NULL) return -1; - - /* Other */ - msr->tcache = apr_hash_make(msr->mp); - if (msr->tcache == NULL) return -1; - - msr->matched_rules = apr_array_make(msr->mp, 16, sizeof(void *)); - if (msr->matched_rules == NULL) return -1; - - msr->matched_var = (msc_string *)apr_pcalloc(msr->mp, sizeof(msc_string)); - if (msr->matched_var == NULL) return -1; - - msr->highest_severity = 255; /* high, invalid value */ - - msr->removed_rules = apr_array_make(msr->mp, 16, sizeof(char *)); - if (msr->removed_rules == NULL) return -1; - - - -File: apache2/modsecurity.h -=================================================================== -[brian] Suggestion @ 63 - -Should probably use STRINGIFY and define the numeric value. - -#define MODSEC_VERSION_MAJOR "2" -#define MODSEC_VERSION_MINOR "5" -#define MODSEC_VERSION_MAINT "0" -#define MODSEC_VERSION_TYPE "rc" -#define MODSEC_VERSION_RELEASE "1" - - -[brian] Optimization @ 363 - -Need to re-test implementing this as just a table. - -/* data cache */ - apr_hash_t *tcache; - - - -File: apache2/msc_geo.c -=================================================================== -[brian] Suggestion - -Check portability due to endianness. It seems that the DB values are assumed to be in host order. Perhaps the compiled DB is little-endian and we need to compensate? - - -[brian] Suggestion @ 153 - -Fix TODO. - -offset = -3; - apr_file_seek(geo->db, APR_END, &offset); - /* TODO check offset */ - - -[brian] Suggestion @ 176 - -Fix TODO. - -rc = apr_file_read_full(geo->db, &buf, 1, &nbytes); - /* TODO: check rc */ - geo->dbtype = (int)buf[0]; - - -[brian] Suggestion @ 325 - -Fix TODO. - -apr_file_seek(geo->db, APR_SET, &seekto); - /* TODO: check rc */ - rc = apr_file_read_full(geo->db, &buf, (2 * reclen), &nbytes); - - -[brian] Suggestion @ 374 - -Fix TODO. - -apr_file_seek(geo->db, APR_SET, &seekto); - /* TODO: check rc */ - rc = apr_file_read_full(geo->db, &cbuf, sizeof(cbuf), &nbytes); - - - -File: apache2/msc_geo.h -=================================================================== -[brian] Suggestion @ 16 - -This value may not be portable based on endianness. The algorithm compares it to the IP address as int in host order. - -#define GEO_COUNTRY_OFFSET 0xffff00 - - - -File: apache2/msc_logging.c -=================================================================== -[brian] ProgramLogic @ 235 - -This allows an empty string as a valid part. This misvalidates "ctl:auditLogParts=+", etc. - -is_valid_parts_specification - - -[brian] Suggestion @ 432 - -apr_dir_make_recursive will attempt to create the dir straight away and if that fails keep backing off a dir until it can start creating, so I see no need to cache. Besides, what happens if you cache, then someone deletes the path from outside apache? - -/* IMP1 Surely it would be more efficient to check the folders for - * the audit log repository base path in the configuration phase, to reduce - * the work we do on every request. Also, since our path depends on time, - * we could cache the time we last checked and don't check if we know - * the folder is there. - */ - rc = apr_dir_make_recursive(entry_basename, CREATEMODE_DIR, msr->mp); - - - -File: apache2/msc_multipart.c -=================================================================== -[brian] Irrelevant @ 18 - -Remove code? - -#if 0 -static char *multipart_construct_filename(modsec_rec *msr) { - char c, *p, *q = msr->mpd->mpp->filename; - char *filename; - - /* find the last backward slash and consider the - * filename to be only what's right from it - */ - p = strrchr(q, '\\'); - if (p != NULL) q = p + 1; - - /* do the same for the forward slash */ - p = strrchr(q, '/'); - if (p != NULL) q = p + 1; - - /* allow letters, digits and dots, replace - * everything else with underscores - */ - p = filename = apr_pstrdup(msr->mp, q); - while((c = *p) != 0) { - if (!( isalnum(c)||(c == '.') )) *p = '_'; - p++; - } - - return filename; -} -#endif - - -[brian] ProgramLogic @ 52 - -The multipart C-D header is case insensitive (rfc 2183), so we should probably use strncasecmp() here. - -/* accept only what we understand */ - if (strncmp(c_d_value, "form-data", 9) != 0) { - return -1; - } - - -[brian] Suggestion @ 75 - -This allows for a name of "", so maybe check for name[0] == '\0' and return an error. Although we catch this later on as an unrecognized name and return -10. - -start = p; - while((*p != '\0')&&(*p != '=')&&(*p != '\t')&&(*p != ' ')) p++; - if (*p == '\0') return -4; - - name = apr_pstrmemdup(msr->mp, start, (p - start)); - - -[brian] Suggestion @ 106 - -I think this is wrong. RFC 822 defines a quoted-string as <"> *(qtext/quoted-pair) <"> and quoted-par being able to quote any CHAR. - -/* only " and \ can be escaped */ - if ((*(p + 1) == '"')||(*(p + 1) == '\\')) { - p++; - } - else { - /* improper escaping */ - - /* We allow for now because IE sends - * improperly escaped content and there's - * nothing we can do about it. - * - * return -9; - */ - } - - -[brian] Suggestion @ 122 - -For a quoted value we just include everything until the end quote. The field values should be US-ASCII 'qtext' or 'quoted-pair' (RFC 822) or escaped using RFC 2047. - -if (*p == '"') { - *t = '\0'; - break; - } - - -[brian] Suggestion @ 143 - -RFC 2045 defines an 'attribute' as "ALWAYS case-insensitive", so these should be strncasecmp() - -if (strcmp(name, "name") == 0) { - if (msr->mpd->mpp->name != NULL) return -14; - msr->mpd->mpp->name = value; - - if (msr->txcfg->debuglog_level >= 9) { - msr_log(msr, 9, "Multipart: Content-Disposition name: %s", - log_escape_nq(msr->mp, value)); - } - } - else - if (strcmp(name, "filename") == 0) { - - -[brian] Suggestion @ 722 - -Cannot find anything that supports that this is or is not allowed. - -/* Flag for whitespace after parameter name. */ - - -[brian] Suggestion @ 735 - -Cannot find anything that supports that this is or is not allowed. - -/* Flag for whitespace before parameter value. */ - - - -File: apache2/msc_util.c -=================================================================== -[brian] Suggestion @ 24 - -Be more consistent in naming. - -#define VALID_HEX(X) (((X >= '0')&&(X <= '9')) || ((X >= 'a')&&(X <= 'f')) || ((X >= 'A')&&(X <= 'F'))) -#define ISODIGIT(X) ((X >= '0')&&(X <= '7')) - - -[brian] Suggestion @ 1186 - -Fix TODO. - -char *resolve_relative_path(apr_pool_t *pool, const char *parent_filename, const char *filename) { - if (filename == NULL) return NULL; - // TODO Support paths on operating systems other than Unix. - if (filename[0] == '/') return (char *)filename; - - return apr_pstrcat(pool, apr_pstrndup(pool, parent_filename, - strlen(parent_filename) - strlen(apr_filepath_name_get(parent_filename))), - filename, NULL); -} - - - -File: apache2/msc_xml.c -=================================================================== -[brian] Suggestion @ 27 - -Remove #if 0'd code? - -#if 0 -static void xml_receive_sax_error(void *data, const char *msg, ...) { - modsec_rec *msr = (modsec_rec *)data; - char message[256]; - - if (msr == NULL) return; - - apr_snprintf(message, sizeof(message), "%s (line %d offset %d)", - log_escape_nq(msr->mp, msr->xml->parsing_ctx->lastError.message), - msr->xml->parsing_ctx->lastError.line, - msr->xml->parsing_ctx->lastError.int2); - - msr_log(msr, 5, "XML: Parsing error: %s", message); -} -#endif - - - -File: apache2/pdf_protect.c -=================================================================== -[brian] Suggestion @ 26 - -Change from TODO to ENH. - -// TODO We need ID and REV values for the PDF XSS alert. - -// TODO It would be nice if the user could choose the ID/REV/SEVERITY/MESSAGE, etc. - - -[brian] Suggestion @ 217 - -Change from TODO to ENH. - -// TODO Should we look at err_headers_out too? - - -[brian] Suggestion @ 244 - -Change from TODO to ENH. - -// TODO application/x-pdf, application/vnd.fdf, application/vnd.adobe.xfdf, - // application/vnd.adobe.xdp+xml, application/vnd.adobe.xfd+xml, application/vnd.pdf - // application/acrobat, text/pdf, text/x-pdf ??? - - -[brian] Missing @ 472 - -Add the missing alert. - -// TODO Log alert - - - -File: apache2/re.c -=================================================================== -[brian] Missing @ 47 - -Need to log on failure. - -var = msre_create_var(ruleset, telts[i].key, telts[i].val, NULL, error_msg); - if (var == NULL) return -1; - - -[brian] Suggestion @ 297 - -Should replace with isvarnamechar() if possible. - -while((*p != '\0')&&(*p != '|')&&(*p != ':')&&(*p != ',')&&(!isspace(*p))) p++; /* ENH replace with isvarnamechar() */ - - -[brian] Irrelevant @ 608 - -Does not appear to be used anywhere. - -/** - * Destroys an engine instance, releasing the consumed memory. - */ -void msre_engine_destroy(msre_engine *engine) { - /* Destroyed automatically by the parent pool. - * apr_pool_destroy(engine->mp); - */ -} - - -[brian] Optimization @ 1150 - -tags set to NULL would be a bit better as it would stop apr_pstrcat() earlier, but tags *must* remain last or wierd results. - -char *tags = ""; - - -[brian] Suggestion @ 1179 - -Implement TODO. - -//TODO: restrict to 512 bytes - - -[brian] Optimization @ 1528 - -This causes two loops through the action list. Perhaps there is a more performant way to do these at the same time? Maybe split into two lists? - -/* Perform non-disruptive actions. */ - msre_perform_nondisruptive_actions(msr, rule, rule->actionset, mptmp); - - /* Perform disruptive actions, but only if - * this rule is not part of a chain. - */ - if (rule->actionset->is_chained == 0) { - msre_perform_disruptive_actions(msr, rule, acting_actionset, mptmp, my_error_msg); - } - - - -File: apache2/re.h -=================================================================== -[brian] Irrelevant @ 86 - -Does not appear to be used anywhere. - -void DSOLOCAL msre_engine_destroy(msre_engine *engine); - - -[brian] Suggestion @ 148 - -Why not stored in op_param_data like @rx, etc. The param_data is used w/exec action for lua. - -/* Compiled Lua script. */ - msc_script *script; - - - -File: apache2/re_actions.c -=================================================================== -[brian] Optimization @ 170 - -This implementation comment needs to be coded as many string operators now attempt to resolve macros. - -/* IMP1 Duplicate the string and create the array on - * demand, thus not having to do it if there are - * no macros in the input data. - */ - - data = apr_pstrdup(mptmp, var->value); /* IMP1 Are we modifying data anywhere? */ - arr = apr_array_make(mptmp, 16, sizeof(msc_string *)); - if ((data == NULL)||(arr == NULL)) return -1; - - -[brian] Irrelevant @ 209 - -This #if 0'd out code should be removed. - -/* Removed %0-9 macros as it messes up urlEncoding in the match - * where having '%0a' will be treated as %{TX.0}a, which is incorrect. - * */ -#if 0 - else if ((*(p + 1) >= '0')&&(*(p + 1) <= '9')) { - /* Special case for regex captures. */ - var_name = "TX"; - var_value = apr_pstrmemdup(mptmp, p + 1, 1); - next_text_start = p + 2; - } -#endif - - -[brian] Suggestion @ 257 - -Should log a level 9 msg here. - -} else { - /* We could not identify a valid macro so add it as text. */ - part = (msc_string *)apr_pcalloc(mptmp, sizeof(msc_string)); - if (part == NULL) return -1; - part->value_len = p - text_start + 1; /* len(text)+len("%") */ - part->value = apr_pstrmemdup(mptmp, text_start, part->value_len); - *(msc_string **)apr_array_push(arr) = part; - - next_text_start = p + 1; - } - - -[brian] Optimization @ 276 - -Use apr_array_pstrcat(msr->mp, arr, NULL) instead? - -/* If there's more than one member of the array that - * means there was at least one macro present. Combine - * text parts into a single string now. - */ - if (arr->nelts > 1) { - /* Figure out the required size for the string. */ - var->value_len = 0; - for(i = 0; i < arr->nelts; i++) { - part = ((msc_string **)arr->elts)[i]; - var->value_len += part->value_len; - } - - /* Allocate the string. */ - var->value = apr_palloc(msr->mp, var->value_len + 1); - if (var->value == NULL) return -1; - - /* Combine the parts. */ - offset = 0; - for(i = 0; i < arr->nelts; i++) { - part = ((msc_string **)arr->elts)[i]; - memcpy((char *)(var->value + offset), part->value, part->value_len); - offset += part->value_len; - } - var->value[offset] = '\0'; - } - - -[brian] Missing @ 402 - -Implement. Need to check if Apache will return an invalid status code - -/* status */ -static char *msre_action_status_validate(msre_engine *engine, msre_action *action) { - /* ENH action->param must be a valid HTTP status code. */ - return NULL; -} - - -[brian] Missing @ 422 - -Implement. - -/* pause */ -static char *msre_action_pause_validate(msre_engine *engine, msre_action *action) { - /* ENH Validate a positive number. */ - return NULL; -} - - -[brian] Missing @ 434 - -Implement as a valid URI check with apr_uri_parse()? - -/* redirect */ - -static char *msre_action_redirect_validate(msre_engine *engine, msre_action *action) { - /* ENH Add validation. */ - return NULL; -} - - -[brian] Missing @ 465 - -Implement as a valid URI check with apr_uri_parse()? - -/* proxy */ - -static char *msre_action_proxy_validate(msre_engine *engine, msre_action *action) { - /* ENH Add validation. */ - return NULL; -} - - -[brian] Irrelevant @ 507 - -I do not see a need to validate beyound what is already done in the init function. - -msre_action_skip_validate -msre_action_skipAfter_validate - - -[brian] Missing @ 570 - -Implement. - -/* phase */ - -static char *msre_action_phase_validate(msre_engine *engine, msre_action *action) { - /* ENH Add validation. */ - return NULL; -} - - -[brian] Suggestion @ 612 - -Probably should also calc length and validate a length > 0 instead of just checking NULL. Other checks would benefit from checking a length as well, so no harm in calculating that. - -if (value == NULL) { - return apr_psprintf(engine->mp, "Missing ctl value for name: %s", name); - } - - -[brian] Irrelevant @ 708 - -Why register init() if we do not use it? - -static apr_status_t msre_action_ctl_init(msre_engine *engine, msre_actionset *actionset, - msre_action *action) -{ - /* Do nothing. */ - return 1; -} - - -[brian] Optimization @ 774 - -TODO needs looked into. - -if (strcasecmp(name, "auditEngine") == 0) { - if (strcasecmp(value, "on") == 0) { - msr->txcfg->auditlog_flag = AUDITLOG_ON; - msr->usercfg->auditlog_flag = AUDITLOG_ON; - } - - if (strcasecmp(value, "off") == 0) { - msr->txcfg->auditlog_flag = AUDITLOG_OFF; - msr->usercfg->auditlog_flag = AUDITLOG_OFF; - } - - if (strcasecmp(value, "relevantonly") == 0) { - msr->txcfg->auditlog_flag = AUDITLOG_RELEVANT; - msr->usercfg->auditlog_flag = AUDITLOG_RELEVANT; - } - - msr_log(msr, 4, "Ctl: Set auditEngine to %d.", msr->txcfg->auditlog_flag); // TODO - - return 1; - } else - - -[brian] Suggestion @ 790 - -Not positive why the TODO here. Perhaps for a decision as to log and/or at what level? - -msr_log(msr, 4, "Ctl: Set auditEngine to %d.", msr->txcfg->auditlog_flag); // TODO - - -[brian] Missing @ 855 - -Should log an internal error here. - -else { - /* ENH Should never happen, but log if it does. */ - return -1; - } - - -[brian] Suggestion @ 1152 - -Probably should use apr_strtoi64 where we can tell if there was an error in conversion since we are potentially taking a value from a macro expansion. Also may want to look for overflow. - -value += atoi(var_value); - - -[brian] Suggestion @ 1288 - -Not sure why we would not want to deprecate a TX var. Further rules could use this even if TX is not persisted. - -/* IMP1 Add message TX variables cannot deprecate in value. */ - - -[brian] Suggestion @ 1383 - -The timeout is hardcoded to 3600. The docs state TIMEOUT is read-only, but this is not true. So, you can modify TIMEOUT. - -/* IMP1 Is the timeout hard-coded to 3600? */ - - -[brian] Suggestion @ 1555 - -We already have support for relative filenames, but cannot get to this data from here. This needs solved by passing more data to the validate function (cmd_parms rec). Maybe need a warning here stating we do not support them yet, or it might be confusing to users that we do not here but do elsewhere. - -/* TODO Support relative filenames. */ - - -[brian] Suggestion @ 1557 - -Not sure using an extension is a good idea here. Better I think would be to specify a type: "exec:[type=]/path/to/file" as in "exec:lua=/path/to/script" and make param_data a script_rec with a type and value. Also we use the abstract param_data here vs using a specific field as in SecRuleScript. - -/* Process Lua scripts internally. */ - if (strlen(filename) > 4) { - char *p = filename + strlen(filename) - 4; - if ((p[0] == '.')&&(p[1] == 'l')&&(p[2] == 'u')&&(p[3] == 'a')) { - /* It's a Lua script. */ - msc_script *script = NULL; - - /* Compile script. */ - char *msg = lua_compile(&script, filename, engine->mp); - if (msg != NULL) return msg; - - action->param_data = script; - } - } - - -[brian] Suggestion @ 1578 - -This assumes lua is the only type (which it is now), but should be re-writen with a script_rec stored in param_data. - -if (action->param_data != NULL) { /* Lua */ - msc_script *script = (msc_script *)action->param_data; - char *my_error_msg = NULL; - - if (lua_execute(script, NULL, msr, rule, &my_error_msg) < 0) { - msr_log(msr, 1, "%s", my_error_msg); - return 0; - } - } else { /* Execute as shell script. */ - - - -File: apache2/re_operators.c -=================================================================== -[brian] Missing - -Need more unit tests for operators. Start with new operators. - - -[brian] Suggestion @ 246 - -Use resolve_relative_path() instead? Maybe a config_relative_path() to just get the path? - -/* Get the path of the rule filename to use as a base */ - rulefile_path = apr_pstrndup(rule->ruleset->mp, rule->filename, strlen(rule->filename) - strlen(apr_filepath_name_get(rule->filename))); - - -[brian] Missing @ 310 - -Need to check return code and log an error on failure. - -acmp_add_pattern(p, buf, NULL, NULL, strlen(buf)); - - -[brian] Missing @ 315 - -Need to check return code and log an error on failure. - -acmp_prepare(p); - - -[brian] Optimization @ 379 - -See if apr_strmatch is faster. - -msre_op_within_execute - - -[brian] Optimization @ 442 - -See if apr_strmatch is faster. - -msre_op_contains_execute - - -[brian] Optimization @ 506 - -See if apr_strmatch is faster. - -msre_op_containsWord_execute - - -[brian] Missing @ 1330 - -Need an error_msg set for lua execution error. - -rc = lua_execute(script, target, msr, rule, error_msg); - if (rc < 0) { - /* Error. */ - return -1; - } - - -[brian] Missing @ 1477 - -@validateurlEncoding does not output VAR name nor offset in error_msg on match. - -msre_op_validateUrlEncoding_execute - - -[brian] Missing @ 1885 - -@m operator is not documented. This does the same as @contains, so it was suggested earlier to use the @m algorithm for contains (if faster) and drop @m. - -/* m */ - msre_engine_op_register(engine, - "m", - msre_op_m_param_init, - msre_op_m_execute - ); - - - -File: apache2/re_tfns.c -=================================================================== -[brian] Optimization @ 77 - -No need to set this on all. Only set it once when we find the first non-space char. - -(*rval)[i] = '\0'; - - - diff --git a/review/review-summary.pl b/review/review-summary.pl deleted file mode 100755 index 799f1672..00000000 --- a/review/review-summary.pl +++ /dev/null @@ -1,50 +0,0 @@ -#!/usr/bin/perl -use strict; -use XML::Simple; -#use Data::Dumper; - -my $REVIEW = shift @ARGV; -my %ISSUES = (); - -unless (defined $REVIEW) { - print STDERR "Usage: $0 [ ... ]\n"; - exit 1; -} - -# XML => hashref -my $review = XMLin( - $REVIEW, - KeepRoot => 0, - KeyAttr => { ReviewIssue => "+id" }, - ContentKey => "value", - SuppressEmpty => undef, -); -#print Dumper($review); - -# Reorg hashref to be only open issues by filename -for my $rec (values %{$review->{ReviewIssue} || {}}) { - my $key = defined($rec->{File}->{value}) ? $rec->{File}->{value} : ""; - push @{$ISSUES{$key}}, $rec if ($rec->{Status} =~ m/\.open$/); -} - - -# Write report -for my $fn (@ARGV ? (@ARGV) : (sort keys %ISSUES)) { - print "File: $fn\n"; - print "===================================================================\n"; - for my $r (sort { $a->{File}->{line} <=> $b->{File}->{line} || $a->{ReviewerId} cmp $b->{ReviewerId} } @{$ISSUES{$fn} || []}) { - (my $type = $r->{Type}) =~ s/^.*\.([^\.]+)$/$1/; - $type = ucfirst($type); - (my $res = $r->{Resolution}) =~ s/^.*\.([^\.]+)$/$1/; - my $line = ($r->{File}->{line} and $r->{File}->{line} > 1) ? " @ $r->{File}->{line}" : ""; - my $summary = $r->{Summary} ? "\n$r->{Summary}\n\n" : ""; - my $desc = $r->{Description} ? "$r->{Description}\n\n" : ""; - - print << "EOT"; -[$r->{ReviewerId}] $type$line -$summary$desc -EOT - } - print "\n"; -} -