From a614afb557a6a6727360d92b8bc6e234bb8a297f Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 29 Oct 2024 08:34:21 +0800 Subject: [PATCH 1/2] fix: CMS content type check (#35) **What this PR does / why we need it**: Previously, the CMS package was generically designed for RFC 5652. However, since the CMS package will not be exported and we only need to care about RFC 3161, we can restrict the CMS content type to TSTInfo (id-ct-TSTInfo) to fail fast. **Which issue(s) this PR resolves** *(optional, in `resolves #(, resolves #, ...)` format, will close the issue(s) when PR gets merged)*: Resolves #33 **Please check the following list**: - [x] Does the affected code have corresponding tests, e.g. unit test? - [ ] Does this introduce breaking changes that would require an announcement or bumping the major version? - [ ] Do all new files have an appropriate license header? --------- Signed-off-by: Junjie Gao --- internal/cms/signed.go | 15 ++++++++++----- internal/cms/signed_test.go | 7 ++++++- .../TimeStampTokenWithInvalidContentType.p7s | Bin 0 -> 7440 bytes token.go | 2 +- token_test.go | 2 +- 5 files changed, 18 insertions(+), 8 deletions(-) create mode 100644 internal/cms/testdata/TimeStampTokenWithInvalidContentType.p7s diff --git a/internal/cms/signed.go b/internal/cms/signed.go index a2e5d57..fca1c07 100644 --- a/internal/cms/signed.go +++ b/internal/cms/signed.go @@ -248,14 +248,19 @@ func (d *ParsedSignedData) verifySignedAttributes(signerInfo *SignerInfo, chains // verify attributes if present if len(signerInfo.SignedAttributes) == 0 { - if d.ContentType.Equal(oid.Data) { - return nil, nil - } - // signed attributes MUST be present if the content type of the - // EncapsulatedContentInfo value being signed is not id-data. + // According to RFC 5652, if the Content Type is id-data, signed + // attributes can be empty. However, this cms package is designed for + // timestamp (RFC 3161) and the content type must be id-ct-TSTInfo, + // so we require signed attributes to be present. return nil, VerificationError{Message: "missing signed attributes"} } + // this CMS package is designed for timestamping (RFC 3161), so checking the + // content type to be id-ct-TSTInfo is an optimization for tspclient to + // fail fast. + if !oid.TSTInfo.Equal(d.ContentType) { + return nil, fmt.Errorf("unexpected content type: %v. Expected to be id-ct-TSTInfo (%v)", d.ContentType, oid.TSTInfo) + } var contentType asn1.ObjectIdentifier if err := signerInfo.SignedAttributes.Get(oid.ContentType, &contentType); err != nil { return nil, VerificationError{Message: "invalid content type", Detail: err} diff --git a/internal/cms/signed_test.go b/internal/cms/signed_test.go index 2148991..d7a6588 100644 --- a/internal/cms/signed_test.go +++ b/internal/cms/signed_test.go @@ -160,7 +160,12 @@ func TestVerify(t *testing.T) { { name: "id-data content type without signed attributes", filePath: "testdata/SignedDataWithoutSignedAttributes.p7s", - wantErr: false, + wantErr: true, + }, + { + name: "invalid content type", + filePath: "testdata/TimeStampTokenWithInvalidContentType.p7s", + wantErr: true, }, { name: "an invalid and a valid signer info", diff --git a/internal/cms/testdata/TimeStampTokenWithInvalidContentType.p7s b/internal/cms/testdata/TimeStampTokenWithInvalidContentType.p7s new file mode 100644 index 0000000000000000000000000000000000000000..c92547ff61a8676659ac152ee00b94d9a63fba30 GIT binary patch literal 7440 zcmeI0c|6p6+s9|d7{)TfWXm#SEz56a>`PgaT??`$#=g@SyJAM!voEPIl~O`XA_q}f zO0q;GSyQryko_5TI=6G~^StimxzFqQ=bk@i=DS?q?f3m$@9TE~Bt9k>{6Lg$)Cx10 z4nijJZUQ9URS1{{%>ppP;C4~EU?&p4aWeSw}X=- zN(!YTOD64g1V}J5fCN<|Q)hxe_JBc0-HzXe-tF`K%=g+zBX4J8za9XXk$NE3f4&U% zzh4F#komq0cJW;+Gy&?!C|#{A{>4_2cAE-;yP;l8@@VttbP{bXuJ%{QzTm(6i~u`5UI~c4>;OD~?)A z?etI%-{m`e5BujQ#(>`ccia|_Egj2SP-wyrk_9!bU| z>MlqcFmaAuFt@&4w4N2YddboB>9o5Re>+1!DrHaIOwUae%fn6}kw?8ue!L?wXpy~M zhA}2tg&y~TrC8?y!qhh-oUGMt)il7Fm;MqI?TQ#FNt>0#L-%1glMQ+gMu^Y~e8hhJ zT<>}yn3FsQp#g)yH%Z{D)F+Y!AgFhWj|B|ghC*o|bP)i@PFZB9%mgsN=&6#VrKO{x z2l#gC5l{{&vgEukF$jf}C|(HqW(n$Ry)6t$1PphYQBZ9_3()vY$Y6dz7Qp-*1Ony| zbtMpd{gh>70s{l3UB2sVM@-TVK3+0-Ctn{wH-Zm7SmwJf`PiWlfa)-kf7*k=P+}xN zi~y8=)&Srrshtt21Nu1l`TlW42b_$HAHfgjfb(<3Q8nQr3uygj3W-wq)zr_)!5{DR z_r?xRc)}kCkaoZm0D+yo?uT*#91+M#jwZM94=>HNm9Y6=)E{>|t98nxo&dFj08|-J z1mwxGWK0zB>r@VS&%dANPv-F4Eg-6wpdmrPpm6)oYkh$-nm!&Zy#v~os$cy>K-0C$ zK8XEZQdmtq^4O7dV(nA{B*w5PyxHbeWrOgQ>TIwpgVm%quB%kC_Z*pX)+S<}XLNj{ zY={p76=#vZ&ZE^QMU1>f3c}3gX?yx*f-TG zZZCi+YIsp{(eHiI@W&QM{ZJiN!*@5jE9b{fgnUve6{O=H{*q92*7sb>jBH*!I+wYj z>5%+E=bn0vH-QZIqaZ7w!{d#2$(;Fz$mczJfBG8$e`%fJpO*b7*WOW*_D z{DYE9U-2rRcHa|)Ujb1tS(ZV$#@`+|Z{9z+5nJJZBb`AZm7Qw8bXd8h;7++_SKqZ0 zYzqq^JqM?A4MqCAuBChx03XwBo=#e|E}ULF&5E}xnN~QP{`jCx@V^EgKb=VXF+>u0KBAXCU|ixR4%wNoU;D5xHf0i zj6v9ufQvhrC2xDGcAMevavUBdD#YrC@!VgNElt;4ztUEmz*ctxT<7Xw!?#dmEps80G1N`{xU_=&8 z1T?*_c`$S}CGUvXzOwvD2(hJ#6F6&;A*xS3xY||61#ZPGP z6)%qntvztH`oajJ$cMS=;j-7MtvEjTAz_4zX!8bznVeoqNpB7P0@!w3G5ZeA=m8k@hk9Xlq3ln+ z_ip!I;z@fmjeYdS_-HS0PxF`X=Po8>;LuNqP>Hmo3F)6@DvyMA@d)!rUyrBC4=)eM z|GM^YA-$&=$w9W`G1}WQN1(cdNJh;{MMCkd)uCDatGbiz9 zE=1kn9Mq^gHd1r%)olI#gr|q#8@@b9dS01_>w0)p z_i`4*C4?hGzu&&8;{l{qR#)3>V0uD7?+kJVzvpa#?`xLK#YqM8^c1f9?=NL5a$7vT zd;RaIX-DF;0xsG365jG%Lox?}TJ%E@C#9!N}E;1Zk8#FPD++-`O z&@(&cpHt+c|6JO=swL(b8(>VK_sctTy(hbE;+#RIWGjaU*Q+s0g($e>sCEzKl{d-% z*1JpmmDVyb(rV}C7g7M)Yi*%nv8L6~WZsxlp;{VWTE&Z=tDQr>MXGzE z_?}9AW^B6Tl~8eZ=A_`(q2;_hC7zO^*D@ifvadyje4|pxoV84Mz9!X{#XjCT6Qg_` z%dm_FEsa%?QOP~$iL1=%1?LtX!k6CK;nk04pt`k=JYFMbjH4_of|~N(3x%k$AgvA+ zE;2vivY@W1-nJ>!KjC?HW4HVL=iq|<$&r5&E=mAq7hEt(02ZzI120zpVU+zNnq<&@ z_G@uookq5Vs;)58&zEa6N|MTTo2IN)OCld>q+*fzKH(DwTANB^JQ^!vJDVum$hH$m z!D;=0eO669(X&R%dtoz<&Wx!MEk_3Z(v24jV#ALGInY06JMN|FEMrs^EN*m?Cypng zlADM)-w;>hXeh|5<=U&J8rMj1ev^Eq5)Es7JnDsk-R57dnku~csO~Yz;dsXvuGpb5 zmf)qy8v+Mj`$~t7*Q8Z@nT+a&CG4S7+;*k};?56GCa4yc=Pxg2=(EvzZ6;7++dIg( zz~>f(((e7rD>lk4H~MqngEmK`lIMx9%v0CS*BpmB$PRg=aXt2srznjT3CmZ0nXBMs zH&3e=q6pe7nn(7g&)#fQ%)DxBY{jUv2$Rv%|26-J?$~dt#n>6ChNTGj# zbB4Q@!IYgcE$hF?ZN7LXeu;-Q+Ly?1=ZZgHu5uJDD7QbNM`ZOqZLXN5VStkLK zZzEYcw}$}M)r;*|-K7>UN2Fwk#j!2}dcDRBM;-@6u-ejtgcgh9Yns}@-iT=K)23xk z6?F+$-?L9J`#h3<@8bO;p7r4cWE_awH!?USE2W!9tWJR+L-!Wxpek;7|0hjEsWcJ# z15IE6%WpKH1!$-=*@conCB<%(Bz-XmzayS^1uzxK| zz+jMG(V51`MXQ&%14dc;dnNM5kjaJc)rOEfIj%Sc>|QT?w2-mJVK0VUu9E&y+l`D) z$;(KU#3x_mErS;?5!(sNT=`=ouI+Q-PL`SzEoMqJU5%o%XBF3a1!-6+jr*+a+_lQj z!8`151BEl7GsYPHVTxz2D&=YQ<*wuacei4m_jh97SsZ))G#;bhX1cC8p+d*#&%oNx z7$zb%UnJVjYVzrQ()Kk}+c+hiURpUn{s%4z_KFLY0c4o4#}aTS`H~3eAnx1$cUfMe&>lH^Smk{Z*&lC0heWYnJEY8Cc~zy$gcz1QycG6HATGF^va#WEE0 zV7-T4ycD%$_8TmHJ|T*T+}ku~9se3?*P?e@eA!K9`Q>x*l#e-aICF&}0`fg!Sa}vN zBYkbXqq=TM4KxD7ekL6(F}bbG;g>L=fj}wOmls^0pd3q<#@J1mzpR3$y1IT;6t(lG zU@dK<;+dv!Y@XdL&MM_F^lft8L|Lh5Xvu+Lq!UuX6lHg)5;OY_cqA~Nl4 zK$F`=66X&`q6w(~D^BE3eZ;O){+;X0E19O+dexaAxJJSu}a#{6342{JqtJ3GN(K`cylc=Y$(1lrA zxKXY6TYOuW{c$J$sno1Ubl-!AEjkv5ZP6sy6{^hisJUFg=P!Nz{PP6vu8cOB1eO7K ze#_~?Y0%8OGu%)>>ThkbJ8$JXs@Y|K{r8WTn&bVM9)?5FLQquL&`TD!8c1LjR>DN3 z5dE-bGNQ{GFcm*J4K~d~I6Ah?sFJm!Ne1hwE z5Xs5WrT+>y3(+ZSIlQ8^X%+ATZ1ZjYA5L&r3LHuTN4yDE*UK)oaS5IEu)8Y#tuKp* z<=!Ey>t*es1~(g=8f~_(HA3T<>RGnuHuVBoxl$I-TrKj(?8`39@%J+-_D`ZMyWxpT zse-9+Tz5g&rMG9+D5w=t4);+t{O^+1*I*0CNW$$&+a^i?- zkpv>OLLKRK7T$aPX=R?;;@cZ@w*5&DxD>fcoA5>2Ctt_a4@(-~wUM{II z_;#uFOp-liOj4@4rxa Date: Tue, 29 Oct 2024 08:53:21 +0800 Subject: [PATCH 2/2] fix: add more tsa url sanity check (#37) Signed-off-by: Patrick Zheng --- http.go | 9 ++++++++- http_test.go | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/http.go b/http.go index a45204a..f99493d 100644 --- a/http.go +++ b/http.go @@ -53,9 +53,16 @@ func NewHTTPTimestamper(httpClient *http.Client, endpoint string) (Timestamper, if httpClient == nil { httpClient = &http.Client{Timeout: 5 * time.Second} } - if _, err := url.Parse(endpoint); err != nil { + tsaURL, err := url.Parse(endpoint) + if err != nil { return nil, err } + if tsaURL.Scheme != "http" && tsaURL.Scheme != "https" { + return nil, fmt.Errorf("endpoint %q: scheme must be http or https, but got %q", endpoint, tsaURL.Scheme) + } + if tsaURL.Host == "" { + return nil, fmt.Errorf("endpoint %q: host cannot be empty", endpoint) + } return &httpTimestamper{ httpClient: httpClient, endpoint: endpoint, diff --git a/http_test.go b/http_test.go index 25554bc..e439a9e 100644 --- a/http_test.go +++ b/http_test.go @@ -256,6 +256,24 @@ func TestNewHTTPTimestamper(t *testing.T) { if _, err := NewHTTPTimestamper(nil, malformedURL); err == nil || err.Error() != expectedErrMsg { t.Fatalf("expected error %s, but got %v", expectedErrMsg, err) } + + malformedURL = "invalid" + expectedErrMsg = `endpoint "invalid": scheme must be http or https, but got ""` + if _, err := NewHTTPTimestamper(nil, malformedURL); err == nil || err.Error() != expectedErrMsg { + t.Fatalf("expected error %s, but got %v", expectedErrMsg, err) + } + + malformedURL = "invalid://" + expectedErrMsg = `endpoint "invalid://": scheme must be http or https, but got "invalid"` + if _, err := NewHTTPTimestamper(nil, malformedURL); err == nil || err.Error() != expectedErrMsg { + t.Fatalf("expected error %s, but got %v", expectedErrMsg, err) + } + + malformedURL = "https://" + expectedErrMsg = `endpoint "https://": host cannot be empty` + if _, err := NewHTTPTimestamper(nil, malformedURL); err == nil || err.Error() != expectedErrMsg { + t.Fatalf("expected error %s, but got %v", expectedErrMsg, err) + } } func TestHttpTimestamperTimestamp(t *testing.T) {