Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
@ 2020-11-03  0:03 imeevma
  2020-11-11 21:48 ` Vladislav Shpilevoy
  0 siblings, 1 reply; 15+ messages in thread
From: imeevma @ 2020-11-03  0:03 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

After this patch, the persistent functions "box.schema.user.info" and
"LUA" will have the same rights as the user who executed them.

Fixes tarantool/security#1
---
https://github.com/tarantool/security/issues/1
https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access

 src/box/bootstrap.snap       | Bin 5976 -> 5990 bytes
 src/box/lua/upgrade.lua      |  35 ++++++++++++++++++++++++++++++++++
 test/box-py/bootstrap.result |   4 ++--
 test/box/access.result       |  36 +++++++++++++++++++++++++++++++++++
 test/box/access.test.lua     |  15 +++++++++++++++
 5 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index 8bd4f7ce24216a8bcced6aa97c20c83eb7a02c77..cfab8dc811e91c924108a003e325b22f53ef0ab4 100644
GIT binary patch
delta 5887
zcmV<b769qkF6J(f6@ND_FfB1QEoU+}H8*24VP!W8Np5p=VQyn(Iv_S;IAvxvG&C(Z
zGG;I>G-6~lEjcnZVl6miHDWblHD)j}He?D`Lu_wjYdRo%eF_TIx(m9^7VQAersv^(
zr2qf`001bpFZ}>eCAR?j(WEa)5O7*`Fw8K+3^U9iTceDGFMq(uB9Z5L!vMQ(5sWC2
z!5}MBQj|*8uBh20X+k&l_HfSuC8}OyYf5*0@u&DATWRdhnk(rw6A9@8=mNI_78Hwb
z;QU`fRg;1@ELL-Z3OFkP1)Pmw0?u+40tp_7VCHlHg0CTa9J0;^9-uMcz}K+3Km%XH
zrUDGm68KtR=6^<kuXcEJwn^ZN#>UOpaLrenuOR|#zJ|l*0c*ba&;SJZkPp=yzzz#A
zfE`k)VTbI{Sio>NMKv5YrHn(YVY4`u)4WnS%`fIOIgNk)?%(ey@3-G>7w$GnxVbCb
z!cA<AE6a`RkZ4?uYZ~@l_wZfUH1GPZ!o9A+bzQY^U4NTua9vZqu0VBd=2U9}fLfzj
z>j4O$IR!vx&Hx6G%Ii@ouTB62SXVRwtm_#7*0pH{%_}T(<`t4L^9o<#uMDk@kIbx2
z@0YCE&>pPA^96%rdchzRFWB6TVVydtne%DK?%>gx)6u-dJndkZ+z!&&UN|v3=oDrL
z!Qggq2Y;JioPxm{m|(E!OEB29-Tr54`oCuTzv+dErl(7dix)Nb?=@HE?fzTM{iCSY
zdbV`Y!bR&WTeQZarPWJWTCIeo)vdm=q_q_#t*czox=F<f&^u{WfZjQy0`%$;=$%P4
znjWOtOwW`gUoFX3(<23m<`rei%nOQ?VVj~adVgy|wc0goTcGCc>Y}LjdJ-k8PM~D<
z=t)+yYUIS&<j81bVq`QfY2uYI@jl5C?~y3UX;YRYr_CZHImOCp^N5VrMN~BFGIC_e
zcB8GU2$3gZ#QPydydNUMg@$l0WQg-1LK+A}hz14)(LfFS0fdUmM-UZNj~`T2Kt8<Z
z(SO7H9YEge;L&@I9lhhw(ff@YM4}BGL}HB_L}G5zutA$qL*;F%eb!6{jbO$~@@O!l
z%@{L`J@XAz{>UKi?k;zVYQ<vw#9(fWJ@4Q3XRcq!b^o5pX3o!sJ7TT2>E`beU&(@g
zeqt~;Yv!Ec)pR%}CDy#<NZ6Gu=qCnqgMVqnc)NYq#ia%KCkAuV2S3$~PGzF9N(O{s
zuo!^a?3mN!f)+~XP=g^x4Av$@r1UO&qhA)$EevFcn~6vN-4>bCmQ}jO-!+$NlUP^0
z-9NKzo1$7cXo!zyfth7rj{3c&Up5z$MxKvujCyz!^4-5zwy9bJoBBjeOlG29Q-6;x
zF%W*{-g8p3?)#J6vqx2P;r9GxYS;LY>PB^%UC-3S%#m&2V0EMJ+jb$74NSA`I!rSQ
z%+g=8T18deyAEm<WsQ6n(K{I9XB?T3kf0zxfV?Y}kDwkufPCm-+rQ`0gJx$3(9Dhw
z9^1&bYbVDJnimcoGp`#toU<g_41Xx}vVntkr^byL4gAE<m5NSYs|Ys`y)&c6N+pBF
zN)=<qO0`n`$e_jW6F+AdKYxry7=Gd>t<Tq$EC@fPq?%#C@KdS@Mjx8<i!f$RFTluB
z$1LUS;)}M(g%@pci!Rz?ThxM!UCd&OUBp6*UEIYhvgofBSoF7wEBf1CDu1ka-zlni
zUn!_~n^8<b>w-dxS=SR$e7#u0x627A`fuWi{+V#1|NfU~;(sNW_&<py{%<0o#Q#Sm
z@qg0@B(R!A9I<{Pj9C8=MXZ1QLJ;wP5JUVAgb@EXe+VLfAArcehad9Kp9ddkcy{Pv
zhDQe;ioTAjiM<W#j-o}6Cx3?>yvIQY?{CP#H*LUy)>Xp|vo0EJSpJ+tyYHHzhVEsc
zq5BwS=)U`>3{n0ohA0oiGs;K#7rwlA0S50|c)@!XT=0H{7Q9!11>a0z1zH|HD$Mfi
zNg>^;=j!QJQFQw}DDc_-ggx7vfWr3MmvDmiB$%N62qkE*y$B>^uYYHR?DL3_efC~@
z+Kwkr+wXX_z3q1Jux<!<SQi94tjoGxA!PRh5VBjRE4$gH^FeQ&b$Z*R)7##1If?C7
zlh|r;5nE!LwZ+tGJzr*hd4BXxC6gzYoFkQ-ACo|ig#>aBjzA9NbFh8~9{kV2oc}lI
zpliTE?-^{+I|drCa(^;l*$XgW**J2B#H$Oe;tYvHq6JsQA{?t?Rb-a925aW#bi_1<
zyqc2@iD#s_HSKIT-EWCyY@ZItgHqxc>zCaP>Woi`e}on2aK0Sw$Asp_^tSL5j^{(-
zSddl)%as`xT2j#rDN0gO6r>r_41_srYQNm^4WB)GG;b^5Z-0<Er}oPoVQr9aDP8+l
zij=7q7y`yWU_Z!=v>G)QJmQ~zAe*sDK$VIz@c=#n!UTYw0B}Nsy`1r4W~s~)nWbNq
zGN$B%DfND;;3)Bu#w0-{?T~axPe{2VF-KwE9N~<djKI7x`Xakk$(o96RYgohH6kxW
zT~?o0BV3AXDSx7+NR}ZiLrX(Sv6#>mT=ixT7P2B#1qc-&G>m6KNwBIneDoW&`Kx53
zb^k~h5Pn|Cija6d@-g<@TbHPR7Mqw(cd=`~^g_P>*B<5V_FjAR1FUAsk_l%Ee<>_5
zbB+3~rM4;Z`{PxLIx!n%i)~Y_thz66v(lf5$t|vXc7L-}cCV?N&v)+M^EO3?LDY#=
zDA`3OyXIPm`pvavMF;zyB&+n#DS!J%vv3$EaD>9$Kgz^TM=!GK`<G2oCiY@f+}&Mj
zBGl|I?(Syw%;t1w{Zyv`wp&`apXV-I7I$~iBvxW#)wO?%s1heFS^t}@y7n)c#7Igk
zNckHqqJK(!l%(s=X7u~aa{a7(y3|Bsn=V^-N-tS9rzp$5T-8%8Q03X9esiV!`^`78
z>#tSR?>b(mX-3&7Pv>W$?m}VC^1Q!a_od$~O3XdGTBzpxzisC-sx1&H%&(AVmid0|
zf53i7`5Q!&*a*p05nW=LtY<g5k5^M#<r`JvqJJVeMN|tD9r;;Bo!AHw&zfypAe$QX
zn&ve5d;UH-mH8$fyB6xt_tMqhBjs@YMrn^`;UY%j6wxIfD)KY2w?U;av%%!&Z=0ej
zt9NjVZz0#pIH(c}1sKK#KYuU{B3J@P!oXm7`S~Ii#*3vclDIh9LZ$^#7DHHIAXmhK
z@P9LDg{=xEDr!@Bk)lf!T%g$Ugcc{VG=YVQr6rV<xRQhwB&r-i#RysvGa{0PKoVj~
z5K@4M@&gnfp7h{^hoc>gbZC+T6C9S@pu~nGH6WouHbapNM`9p)8u=;$1CbYoxG>a$
z5DP+92%-Xz6oUBt*y96*9wz{qz@vm6B!B1_A%_S!^xVi}!>a}!8h2*ckr6@+5PrI1
zAK)0ak}2V5pFVwJz;uLQ+IL;t{AD_0#9-J;rqt}(eSH!i;Yv5_>b`}4wlv#zWV%`4
z>nd@~N`{1=d=u52#MnxPl(j_|TwSAQYhu2nPsX6pQbyfJa>YpL;#wRbDX%_?n}63d
zi%r-5MSibeVG_fvWJt`~EtkZ$WsixOl({jzW7Qj#>!Ks0BQ>|I(eK*(TvT+V)iyPv
z!b9zsJK|dMZ@#@CSO2<B+~xSS5dHakl#TLe6LZ1R{g`-*2TH7E&dv#!yNSW&v~ZRV
znsmQiZ;7#R(CgiDO1!jzuXf{sS$|?B4R$>wzLJ5h#`BrEEwL30d_A0*n-W)%!@H5W
zB{7u?dbeC{XJcYooZ8b7&#rJ!N1SA}<K56OF_HziT<)h+;wf;Lb~z?3ETxJ}?TDki
zsrh!kB!*JKuXn^xDBz9hA+eJQppy7Vf*ck$ione6h?_vrW2zl7lLy4$a(_s?#DOGM
z(tz%_v!P{nyeyoA0pHDM+i_te3+$ebSf;>km-~{$MG4fLY+Lw<8<cnR*=oF+ldTRW
zY6C`x%i(UeB{sqace5dJku^`dCS8rE#6%Ft<$k&^I3*r(K$%@GiH8ihK|%eJSV&Ei
z?ziJ5v5*3MxZVzmiGxr%`G3IZZaF0eVhoysdaC()AK)KSs84s}@qRWg?4$UF=j$<X
zkN4)QH8C;Jf5Q2Yn1`=5A@PoWrq-mZ;c~w)Sy)GuuV&NjY$()k#{<KKbGW`9)32w*
zI9gjR2X*5u@eS_V{l+A*4We#`>+x;@hq$PNgDaX+h-Lr)06+i$Gm~u$C@y3bNI*a^
z;F@d!N-89{mjngYlAyp^66`hYC5hv1$P_qyInJ7z7qE1M?Z3BZlg<oBf0L?Y1vDKY
z^J^{AVT<vPil@s*gVN+`#`wC()Z37e!Gowmuo*QQO(9Ih2(^x-Qe+e-V$2`UCn_r}
z#rNwv=*MsdLt?<ZIL?0!JU91vTs#m`TajCq({%iv`$wzO9VMXWHg%dQMB;V%(Y{Yv
zp>s-&b4F3;I7sagkfFh5e*wzfb&X&9-}(?bFDiiN0hL8W(-S8TCQE4Y9W7u=JRA;9
z#vm|7Q8lht6cvuGbFffQB>6LN99j{|k$_x;J&=~7y}P=AxK09c==qjZ(T6i@N63S8
z4pNS;<m2Q9uLy+(=B~v&EonhSc)S)gOnfeNQlL8RI4PS&@h%j%e}fJqGl|{3nT298
zp>+{nr!E0M;B4(Q6N0+%YeO}Oan9H7#JTA4Xr(B38|#Wl3a)giPYxZ{+j87Ks;-q+
z)BvbH-W<F*L8{)c{Y8n_VwLMh*7;b5u<1%P?kn~Fm|Xa^lQ)6Ui}6BZACZetJaq!T
zzHf;yAK~`XQ%ru>f4e2dAP#*z&6Mf6U*hZj1_rm8=^Po)YOe8{XdjKCJYs#^&pj{l
zMx<Nz^b48lBtdrjX3G{B2&|F3HfBq0<E+&Xf9M$^CF0+8coX0nq%zoV>U1$fRVpjM
z_Lz?SQiv+*>CtoYL6;t%_)ywk6g8gHkox1#3K25|)k8M;f4v=u?>;{A<pomjhAuWZ
z&S-0p@rxkzc(UO|P-~pY&?1N>LmqNZ#w{TzZ|w&^k#uStZy2IDjJ^W?6aA7P^i1zz
z^nolPs9NnuVBn0Rn<wKIiHV@}c$49U1i{ne$z}uOk+~Try#<DhIZSG}97Jlu=6hTm
zXW%i<{44irf4vfnrW^auD(9DP07aP)s=}b+;8T5$6u0k@0Y5(F%fN`$vYv|3RB%22
z;+k2^q;Kna;jckmh7LUwD3vY0Vii|0_^)EI?3&z1GpD-p@d=2`$~@`VRa?JF-e=aE
zN#_V0l10H5PC;q=&463awE>vVUwh>Wyb-M>0uH9tf1Q&)xuBgjS3RmmZ!7>V_BKZg
zMxE4b-kIbD)d$xIVpC=c$EibEZ}89}W9dsu{$|P^f}3~I2d^MMBe*4$sxySXrc0Zz
z#+)^biDy`2h@Kzw)HHU7v?ZN=FR?Qe$fCV*{BO6NuytKVSxJZq|0XlptR{^pH<3X!
zE&D97f2?T61uPT)km4TU_Y-h?>@am{tGXyex4~9uih;1r`fn>8U#E~06B5oHylb5J
z6S)zL<9zUjg8wtcLM0uhn`z#r&28ncOegx#t#jlkzT6hJS`hkhSdO<k#5h)ibUgJJ
z4tJaAlKZYbJBRJ0-#~h^aG@>4QRm{7(C8DUe?$&D^jDcxa2e?`TPlL`)yjE!iQ~Ci
z4q5{%CzbfQ%yG<7F#*PO)PnOOVLH^PRTdRon}jH0Iu>(|jlt4=JH-;2bHqoaox644
zKZ{P7JK-5WAM~T(<OJT~QbGjMLMcNOEMrA&+=PccC7n>N1cLq-v0#p!suUpBxkP!@
zf1XU?)ga)OAI~%+r)UBf{>N`VjiNca2qm?2snvNF*Q(GGfDQX%Pg0L2Ywifaiwity
z$Dlbq9~wVN!khVC*MsnC6FFDY^`=H)e&n>q2+ZN$rFerBw_9AwSpR8h#tawQUr03g
zLxxA<*38(~`h##Mi9s0}F<>x*bx+p?f6T@2lrd5=AdvEbhh+pTI?1J*zLZ@))0gIM
z>Q)C;%19{;3%Uo<4GwU5KCfyFs!3V?<`n%uy3p+)@*8;iBN8CwL(#wJ+%z=V7qOin
zNhu0ZI^c$sQD{<eUxkvVB(cz*MxaE(dQbFtLDdD2Ta$wjy+#XXhFM3?2uh1<e@HDS
z2nVAhNz^g);}+zPookPV=+J5psDLc~TQ~Hk4Z8fCkhhz4Lvvu7ZA7)CL<WR$nZXdc
zNNkvWa^QyesdMUKFg4g35Eh_`f7guuwrfomLL?R&K5C1r>9Z#_%F-g>(JkV^&fACK
zRb;UwR7eN=?Hl^uyoETxu#1Qmf42-{^c7hw34Lii8t=hN8qI1<zw?giwvYGvm^fQ?
zPF}PRJ)xG|po9O`jeOmX56Wz~y~9^}%y!>c)+MyOL#Sb`QS^uZA!4aL8yF&5uvXHr
z$mj!MVN(U^#{}tQv?&+7gnp7)Y~bJLGSk*uR1Cd3YjUCa#M%*2TJJW?Hr(?5EA-{=
zZJ!+-ZTFK{&CI`c0WPAPX)qIFIuNP_d*uvU2QM5-iz-Mn23V&?PpQx^^urdD+Y=>!
z@Pc@t@$S`d9a`-H6$C++68bDP@W+mFC@rHPjTm4yH9RFk|IiK#65A>8kpDpT0ZSJE
z>oC>6U=vbU=>lazWne!#zGxhX-?7DsVG+SMfYMqADH|#y_wji}E<0rZ67w&QC+^-N
z{}Yk_==>r(5xzvr>^39cCyI2Pt5AA>rJK9{G@oepwCztw82CNOG{t{{1j;7yIS{G`
zyXA~s11}yVi$-Re6gw{-njVidA%W3|duQQfq+F0<T-a_sN4FtF9!Go_*~-**<A#C4
z+bVQCk4w=`MjSL^c&%h5l5qDO{aBf!fdKhH1)*^yfvloWtdHh@Msi5U%Z)&Ps|toe
z3A>7XzaP^E82QVKwh%w`nULLJ(lGprsa6ucoFX(%zDuJ0l%V9o<cuN**9wWkur*OF
zFgFaoM9OJIEoX=8MHGPy`tt=Zga=yIKyBzW46h=pDTyKnNB>0>0|w@>(rt;4Z6<s*
zn8<wnHch7QieF?{Q(AqM67qh3?YDW1d~eVT1A&EZrCb+s1X%9mb-d9;#^jg=I%oxz
z0TbN7H4NIg-`t#uwBXFx$LU0g$Q@fL5>1fM{Q)N9Dc1>CR7$}TnL%C9W^nMZ?FPA)
z4F`c`*rBm_nd9eJ76;uX!V1bUxW+Www=BZ$RoW1Pqk}2}YWY1p;C^|3yTsuUwH*Mj
zuu>Ll;!w%pOHS;K$U34((PN1NFDb9S&Z+<Ge~?3jOlOh{Yd{@~A9JvL{nr*IV7;L6
zyxj7o91bVcC-al4{jIE|wM=gb%+F)L#nX3vPIhB%k;V$4kJHifBh_X>{VJ#bN5zkr
z!BaMbp?iaMaJ2!9L(flt>Vi)X3`HU82?Nopy0Ob<`yLlqC+yZoi=#TFBv#g;t09MU
z!VZ?MzXj|6a@yb^hz$mA0z~I;GDS(kH6p`S77)DSN}M0jAlS2dmU<WnSYJNSmIHVs
z0FZ-Ta#@4sI|o{@ryn5z9!o>~aCQ||EN&&_+%ghM5=^FfCp&9@rbBxSVq0UAXCT-6
zHeXJxO$V}2007&~52|qk9|R%h{ZEWuOb$!L?D0xb=@9M&WoNHE=S|)tDKojHId1YE
zNtwwl&2f|WNXkrZsb|u3#^slsZ4E*H70bw$kp8-#V^wO!Hu;<V5uLj81+hS%B8lpi
VoX5$|9>OH$w>10)&<E8Jt?kl7LO%ci

delta 5855
zcmV<579i>7F4!)R6@N1>FfB4UGA(B^G&M3}W-$s$ZgX^DZewLSAT=;#G+{G0GA%M=
zIW{dcW-&4?Vly&0EjTq}F)?CcIAJ$4WC~V8Y;R+0Iv{&}3JTS_3%bn~-~i5DFleEr
z0000004TLD{QywCwg3vzY%fU=XshY~Up&AU5Aemiejv;qX@BA)fyB100S_ca97ZGJ
z<m)X{Qj|*8o~YR*X)1`Vt*5R}<-<Cr7pyd*YsRR0^n#LxdTNVJE#v~{0<r>c{%>RD
zq`(cE<(!}mXC<J3vk^?dS<XTr!2=P{o(@0&Mr4sh_SwJ#H3l32BRUso0F3BVfPq>9
zU<=ULC;-+Hk$=xM34qnqumK~c0b2t`L_iG~k?1@i4HzdHfB+|QBANqOVgUxQ#40o_
zktH4rAQGvnh(zbqk(fnv8maP`cPgLx#(XB9@vq<g`z_`D_S@~l-AWBNcZYkpi|uh`
z`Eex{kLz(g!@lb#zUzACUEg)M*HyT#s~)baQx&djs(;rtsIJbOYE3{;Ycy*;00Fh9
z04VJlzyMQuMNZ}Q34j3WY9@enEd#*1I?0%MhhxgTLosCD;X4G5snz+3q1EvLm9`t&
zg>`yAp)ib3D2(a}oxCvYa~DN>Lh;xwL_T%9+7}q8E&QU}LOS~^2WAVOzHFhF+!k)3
zv&)lEh=21U6gqVg3Y|CH|4mN++iw3mxir=GY_VzKqUQd+?#jH~f2+BF6!m&fmMvJc
zXniG%)>g2zn(0cbm#nn9)m50Zrn02<lqy;~rBDg_XKhN*KWS2eUPT7|Q>j+llSHHK
ziK6tgDgA7Fpd{74o<yO2Ie|iKQ}jh~PN-JBh<|Mx)Vy6?6xCl%o@AAYldK{&$!b=Q
zm>icN9gj+mj>jZRJQ5||A4%f95hOWv&W_~NX=Eg)SvhqUnbo|CjA&j&iZI!Jym<{N
z@<NDsA7qI4L_+vb5YB-RasFdS2Ymq1L7^WysDnLxR8{d1s;b)2qpB*1hxa;mc%LK4
zdw(1{dcTpQcN;i*uW_STq+z32oKd4#%uX3JYB6T0yiK*&qQQ_M%vnkv6=u}=BF3?2
zzJp637{%RP=1$Qq*$W_9jLos<{k#6n^$)r3-#69F0p4;*tkpK%{AFUSnGg_=EXF3y
zoH9MzF4wfgnzuX|!<q>J$zp6Ukc_w6cYkeU!ofhY7@I#hOx-9|E-tNRK{yuk6SQUp
z9Cox(*4SWc;e{-=CQKvBg?$8Za}nvk+a`0`vP<{)yXI4^G7F2h`)8JIQ*;Xp5%tk*
zFthB-QNOwL%VwkU$TO17Q4^6vzWevfHCAb2RKKW;%3RcI>QN>R!eP1htkkUget#wR
zEK=2cxIKTH+CBcHx>22G*Ee-B^W>U0Sly`mwtd)C6Vq(F64T5Ewe;7lR#8>=u7z4f
z*(2XZ6c5HYEY55Qh|rH7KHjy8htQ55K|D0E?ceLzQM;2Ph;|2uj(z0Yy>lZ+?aKxZ
z+E<MmPnuGV794)juu;P^qXta|4u8X8YZaZmP8seXiYLa5wF-uewdzHTwQ8*bf>D#<
zFdRn1I1K1`jNvdGR{H~nH50;NQ&Y|^VmNHd$;F@AbBiu$Pc68}Q^-8^%wmhyh=mrd
z@ro>3V{5bmi*2mpifyFAif!D+DXQqN6jby#iYfZrUnrz_-zTDYUnih=oqtU{QS)NL
z37XduO@O^w!`Dj*Ci+ieiT;sLqW}JnNaBAZkododBmQ?1VZ{GK6!E`P2qLnaK@72e
zAcR=|4?(Pd{XPKke-A(W&w~&DJ9p?Ie;s(pKZhOi&mRXJYI$zRftJSx9E`q>s*Al9
z>W-pDpQnZ!yr;ni?`NpNJAY-Mq2?9C3^XqoWMKZRL%a8SA%^Z<fT4RoFLd91m<v)K
z%#W0BAxz3kc^1IDU!eu>RbavU6jtya1r@wMAqDR=0fm~LJ}1!h@G&9Xs_*LQR#9|&
zJtg$no&-MIk5Iz)+lxSg_92X*JqRLbul)xhWWNW5?Dc$*efD2_+JBx$PuuZ)wY}|k
z^02N4cv!cCJFLsPULIuEP6yeo<CWd)(y`OqXPw?U>GZa@UC(06<t(;aUB$N8YH>BU
zTF;qUV4frWbJ^rdCg;f|=SXFcXC;H217wf~`JAl30SEtUFz5daHs}~=(E9}$^lkwL
ztegv2wgL-SHjW%(5r3<Kt2o3Wu?PptXnb>Jw1Q(ax(Pko(=CZ(oWVKqZn+(hiDB&D
zF4v1%;umWd9xkfPZ;641ZFaezE(dHvbzyv57%JEMC9zDXG{p*Sij^u#MohFPG^Hm@
zN=dX1=A^L!bx1dT`0&xZZNpz-bB+zDL*iD*H<YgZ3k3>Q8-Mfw;~udcW=9&0Dib2{
zPk)fjSS7$pMVoklp8#J1z)t`?nZaDja3RAqhDi()&}u1L`o)%fKvr*-dP-rGqLg?^
zyCkS29TFI%FK&=>M^8sy+#Ge8;jCs(MmH;?rXpJrm!d7J&npovL$VCPGUN)86(T00
zC7Eof3bt~?2Y(My8JgllijNw`G$Ey!m76~LmD>DWs?oatWGo1WVa<$^ct-X)_S{>y
zsK1sPnNGK{YtQsUzW>)A<?Z%fd(;j_6NTA)!-YQ-Hki3a{oYdBH2FOeD@C7}eZs}I
z=~h<Vm$zE!&qXB{*FC#hD!bQI&gVP#?|GY|L?Qabs(+L0CY4=tJw*NP;<}=Pe@&ED
z`sb9t{i9m=3zIj);qD)8;wPjxY4!cfrf3s$IWq3<GA$Wub{lthwR&cAy0w0)&jed8
zt=rFYA2N-*yQmT`DY@#}zeaS4m6ELg%~xIf7ggdUBydvx3XSLzBOU4bvl;z9vt2*y
zo-%c@*ng(X)}_)+mdz>3vM*OPRU1@!_Nd=o>HdE6UF`a674^H0*J+wjR?5@)d8qqv
zn6*6b@7JB_H;Wc?&+Zng`TlR)yN+rbOb+uq<e6>0U;7@h9a8=ZQ6)Yyf@MURcsA?V
zRc<5}l~(yim)OWijuG9$MMZvA(I+NS#It7GHh;*bN4=&wRsNp8S5jrZi^#5r`t!|n
z_4i3RT)$G<qgr?fkvK+_iHU~%T<ooIIn1n3`T5(XsLJXs<l=kC^)e2+#6v=+^fvmV
z4@J7bTyO!}LiR-qrYw}OK)PaPMNt(*RA3)d$b@j1Qi@v?O;FgL;NrxVCbTe-WeF@w
zTz^Tz3KC35EE!Ry2r5KO8A6H>wIFCfDE*M+LzEt%@bF{@Cpt9Afe8*sHyqipqy{B4
zB$)w;3`b%x0>kVDBNvMJDGDP3Hlzh1ECgABNQI#kfS~Z>1Ro{{_36QfM+rMf2pU0$
z2suK)0m6+BZ0xC_hsM{8JTmaaxC6t37k?c9dCBZx8NQk=;jlk{{`kRkhGN=xZQT5A
zK3~XU_-eM~?Am?35*O)8H|y%og@L#<+xBF-+2HFgvFvJ=w1va?E~;6H@zpFzE6Y&0
zx<}92z;sEQgfXM1j=P`af|=6B)d+%8Ui}j{uc?-ruKk<*UcW;nj$O@?nl_wHiGOcQ
zUlTVibzyu+D>o|FMo33UYHr!1-?jJI$OuWRZF(dH2b=P5zPTh<|GH1S1^M+5{rP*8
zmGY<)YsuOHn>Y&xON?a>?<uFlfywE%@Rbdmc0itwiLGeh^Wk($%#^{;hU<A@;w21t
zz9g=4!OhnDk-;%B6$^g892wjaPk)iq!->Huv6KpYIGv4mYvNiQ8x#`LuyRmHtaQce
z;mkB~5}ce)2iz@jls8R0UDFnZVkO3g#82GVe!QO&J81*-A#oE5dSQG?%w&QnB}S4!
zmxYfaAcI5VB@p<UY)Gu+0WdjT5+`v$iIFt01M+U@7+y~cA7S8!``vh5*nh|ZJSZfd
zDZu0Dx+t+x0=B0c7cRmE?cshmThI1%tAmTyc-iH2IoyqjiKxlpZb>{O4HVC5XX`Do
z5Cn8O;I2z<iGv(4hNn|vA_H(yQ9vagLgTap@_I@Pq<~+}$BSy>A5~93FFTxWiG3V}
zsG^{3Kc5}kV+#H4aJ?Sz)?bBr7`ya-z9!!B;(j)#CJy>gxnC0N@HVF;&e6-*oOZUH
z4){e2<B<B<Zadx$jRNv|Ub^rN_vdQ@`j*&6d$Z}Ha6Klj!GAnp7$v4r-0^b09!?As
z&tS>mo{(4uF_UWyIXPS%l>u>Im{u=aX;@$_2@0$wL4j?IX)j3}cSEMY;mdK>)VzSD
zBW(Y@MLTRUPO6d>&~${%ueC^%(hNs`e^fkOJ{puJS2M=fMW)_{j0_$`6@tyE*=P!3
zDn_VvER`aoI1yw1cs@~CVJSaBy7qnyPa7o+*cQk64?y7NzK@CrLT4)y%VC<1-*f+H
zb-JSj^xURSGlfXJE<f7$DXWgQZ#qi+3^s2AzbNzu9zmiF*pXCs7w7xhr$h>Wp_fwu
zGk#E6L^36D@?hISlOK@-ro_pS&}0mQD~hU#+C@=e+9n4Jg(J(KffFGYp&TJ7Mc9M2
z741E$4v6c7poE@p=@fkkb9RJ$N#!8rd@Ua*53q|+C}HVZ%zIf2A|l3aLBr6OQYQu1
zYsX22aTM=JNjvB;n@jBOgK;Q-7DL(;;dOTl_<_jWPBVdM3coh4ON?{T?G9av9?xq<
zxrgDdh$NCq*Zp$nu(3JE?WvktdF2L{3ggYe%?YT&4cl*`crSdp{$`brWsF@{qKWQO
z@24q+U%R~vj9vmeH1-sw2*tY>@b%-I`0^BKKfUAfyFPMaB$Cj_doE9Z&;1f#_ct)O
z%}nRWcvf?b-$eUp4CN8)<9_aWkvAgUvZr6jR3{0t+c#Ubz(8P)<h3zdY8z**hWJCz
z5GfJ=uEU!E*C3U_ep9E58LCoQ0k+3<?3Y4RQBRMalMlM|_{4|O{-UVyoQBjNe^!W?
zA*dd*!SC%reE0E@FE5aPdN*{j!Er`ggN$DUp~sUAFM?X*OokRgEE)2UdopebL3wLG
z_=%)b<9Ndm#bNXn@So_H1fgeo52Fuc2|?9rKLP`16x}=-w@6F`rN^5LFC++_9#1wK
zAdk$=FzGEYWXxew!{s1S6E@%D;y443dFEfaSL>BvG~L*LRyn_ad;=)TgisX*6$hW{
zbELR^j|}+nDPIOgtd{jujHZI?`4`vBVkUiC&kKJI>N0fbnLw#*`4y|Uiot&si)Gj3
zKAJhzm5)zAWLD-$$FAD?P4Ygo-b^}2;E*f|wr~nc+iwQkdaez?eE!-iSKy6kEfH`q
zt?r!k$p!7Kx$04WHF{$KaIv>JS}^LQX7kP@FQ`7aMi84aQ#ei?%6fx`78y%lQt~%b
z_7L2>i#~V-`5D12p;VnA^fg`Dd^P5*VN5*38bkE_n5U+(JESe??0bowp+FYxjpKj2
z?S!rCGRjIqO!zmM$!0ZaM7fC!qG{P@iDgAIE?}AXhZOgJ2*00z+hd2ROIy`NA-WB=
zLQ@QcZPtHV>G(Q@oS2Ys?%-YH#GlBGU>xUzHx&GzDHbZ}Fx^b^Hf?Sze`Pw+hi;uC
zNAcyhu+@Ulhr@Eb)gi{Q8l>Z?zi_zQM3>xm?b$hOC;bM}n}rK)A&xp1uY^XQFeP%>
zp})$kg3CyMm)TMgl&@CK%S#;3)pF1pSUIW0&t;Bdj*1B|rlS^|7YWm$My;}_;Myca
z5!11lb8HNj?%OGr$ebfSBJJF*`~F#U!rTeZ`1zn81t%x)4wn)lkQPcAqF@;-YU3t6
z>?!GlawQP-zla5M>{O)yvCbvRtM+6HuLc3P{CK8+897B0xbQ!I^Jx^#(M2e!rAw{O
zv$$4;mH=$n7kiR=G+A>;2wq&^NjnD3>G{z3NfO@7_qra0SDVPWnyxoB3iBhUHAY|#
z_b$a7q`2MUQpWmEOEYG;(EdWA!5=a_61Qf?zSbXvJ4p=6(1-zp8LWG{E?_Qxr;L%3
z0fCf%4?HX*V9`k~-SnmG@|nIgcT=}Is8U8sVOY>Th;DFz%kz0vV^B@X@;9gG|IvkR
z2a(^v(;txlAs>qVMdzlW$-ao~1W8I!h|&Q!q>Mt7iu)>*JSB;R_A~+|64ra7#|x@1
zfZUoKgy=O|I5W&TdPY!MTtjL(K{yy4NurK_p&z#(f9za)G(?A1dq4$b@!z_kH*L`6
z=Y+i7tQ(pG(`+NEB_%Q-jLQs$&_!ay?2`jG#7~`54}+<})_|}8P5irN^tWAWvJfJ%
z*zi$XTuq-nsZo{|0grAG4|d)@46h=KC80t(*l*v^_vS6c0ft>fw76v$qp!$fN$5*|
z<I#8zUeah*WBQ$UOt*c!*T=-!s&n$9edr0b<OUu5w{GO?c6?A~!|ffu(qp#!#<DJ<
z<sCu|YmK5m{0|XJ?b*N((So&-hDAmn2n(AkNIxb>C!<Zd;3f2v%whxoK9`xc-lAgY
z)mf7Z%_r85h|+quS>~4aU!gB|Z~N?1=xDp2#A;^#wF__&<xGQ_5YvHBE!Zn(*gAOO
zP+C+$nlZpSHF`>gexV<>ARcJEdo^5#R(n7NL6D_{K1&V!v7;PH%P2@A2AE9^Pl?b!
zw1XFu!V@KbSdiFGfrtDDvJY6g09c2q_63`e!b%q?3n~Nq(eXv&K>Us^P7I3(z5$fh
zI!M`28M%+oD{|Q(`<Ix1c|39V7Wtou{72^(*@^HaT4uKy`94vk>s*D>E8X1nr};#)
zr)_^i!ocrIrYZgtBv3Yq&w)@q*ez%58hG&_Su`?#)1=sW@zC^mqzMU(M%+6KCnM#8
z6yw5n>p8j&A@Vrl!^l>qwi`DL6y8>$<9S?)b~56i5yNXGE0Kh|@94+M91R4>2Pz1S
zBMD>`ePVqy|1*+9I$mxBT2(L%O4wE8`~8?Uz{p=-w1xPg&xGs-lZN3}Otq5m<rJZD
z@?8>t?WY7K7ba&EIk;9x6o##dYJs_7@Fh}CBWgK2TrZ*sWYC{4cp*H{vIc5Hr(t*%
zQB6q{IXL<+q8KnRhm~$id~7q}tHDI(>$hn#eOLS<!<y3StCW!UYroB7<a>i&7ziwM
zE9JV7BfxSeuj7p-GA73~&_OGx44B{su3^xB#{K5zOr!;8#y(CbN<{A1N|9)SgzgV8
z8Be)RxS~=DmdFh1f;NMLhix~=wQM*DEW-|s#mgK&$Fex+HW5}(j=?pi*}i2FcCXTg
z7#tl`5m3wT;Q{x{+a(T<sO<oFg_W{c6NgF$Uvgq^MAi{SiXKZGcu9Hnbx!?f|AQQV
zB4j#~Tv!9@Sp1lS<?Fw;FahfYjpyZ-FXeDJp+1?PRPAqNC9P$8OJIH;`z@Zn>vOUj
zbBi=q2z{K6o*$_;3+h)n{XZ&x#0;LYAq?Fctb?l!U>tgWQWt!BU?>VvPZ)?+)s0;?
z+xNJ@I$^gyS{&6WC9$#&T@5*;6LzqFbp0(@|CiGS2SIEwa1$Upf0HRn60Q*$wz7cW
z9arM~hz7x))w9&YK*0L)fwmmLBLRRM^peXOG~YSUf<65R0q|HF;)k=Vuwrp5A?KEn
zP?BIW%{$pyGacGv5ZfA?JOjDjxA}5nZ90&J0sz=<eo&1Y_#g-|?|)+SVscnsB4&?Q
zib{uYCn!66<vDNi9!Z(WEzNP0_ejc2ZfTC2yhl=Ia!WmvrZX<T<ZNpQ`mb0<wuJQ8
p{T!=OE4Ins?2qWwoiB(5`V>i2ujD*VcJ>e^DZi!Rhd$L1t?hiS4~zf+

diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index add791cd7..671e441ca 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -971,6 +971,40 @@ local function upgrade_to_2_3_1()
     create_session_settings_space()
 end
 
+--------------------------------------------------------------------------------
+-- Tarantool 2.7.1
+--------------------------------------------------------------------------------
+
+local function upgrade_to_2_7_1()
+    local _func = box.space[box.schema.FUNC_ID]
+    local _priv = box.space[box.schema.PRIV_ID]
+
+    local datetime = os.date("%Y-%m-%d %H:%M:%S")
+
+    -- Re-create "box.schema.user.info" function.
+    log.info('remove old function "box.schema.user.info"')
+    _priv:delete({2, 'function', 1})
+    _func:delete({1})
+    log.info('create function "box.schema.user.info" with setuid')
+    _func:replace({1, ADMIN, 'box.schema.user.info', 0, 'LUA', '', 'function',
+                  {}, 'any', 'none', 'none', false, false, true, {'LUA'},
+                  setmap({}), '', datetime, datetime})
+    log.info('grant execute on function "box.schema.user.info" to public')
+    _priv:replace{ADMIN, PUBLIC, 'function', 1, box.priv.X}
+
+    -- Re-create "LUA" function.
+    log.info('remove old function "LUA"')
+    _priv:delete({2, 'function', 65})
+    _func:delete({65})
+    log.info('create function "LUA"')
+    _func:replace({65, ADMIN, 'LUA', 0, 'LUA',
+                   'function(code) return assert(loadstring(code))() end',
+                   'function', {'string'}, 'any', 'none', 'none', false, false,
+                   true, {'LUA', 'SQL'}, setmap({}), '', datetime, datetime})
+    log.info('grant execute on function "LUA" to public')
+    _priv:replace{ADMIN, PUBLIC, 'function', 65, box.priv.X}
+end
+
 --------------------------------------------------------------------------------
 
 local function get_version()
@@ -1007,6 +1041,7 @@ local function upgrade(options)
         {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
         {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
         {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
+        {version = mkversion(2, 7, 1), func = upgrade_to_2_7_1, auto = true},
     }
 
     for _, handler in ipairs(handlers) do
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 0876e77a6..ed7accea3 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -4,7 +4,7 @@ box.internal.bootstrap()
 box.space._schema:select{}
 ---
 - - ['max_id', 511]
-  - ['version', 2, 3, 1]
+  - ['version', 2, 7, 1]
 ...
 box.space._cluster:select{}
 ---
@@ -167,7 +167,7 @@ box.space._user:select{}
 ...
 for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end
 ---
-- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none',
+- - [1, 1, 'box.schema.user.info', 0, 'LUA', '', 'function', [], 'any', 'none', 'none',
     false, false, true, ['LUA'], {}, '', '', '']
 ...
 box.space._priv:select{}
diff --git a/test/box/access.result b/test/box/access.result
index 20b1b8b35..92d6453d7 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
 ---
 ...
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:get(257)")
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+...
+session.su('guest')
+---
+...
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:get(257)")
+---
+- error: Read access to space '_space' is denied for user 'guest'
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+- error: User '1' is not found
+...
+session.su('admin')
+---
+...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 3e083a383..89fb34fe6 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -824,3 +824,18 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
+
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:get(257)")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('guest')
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:get(257)")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('admin')
-- 
2.25.1

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
  2020-11-03  0:03 [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions imeevma
@ 2020-11-11 21:48 ` Vladislav Shpilevoy
  2020-12-03  9:54   ` Mergen Imeev
  0 siblings, 1 reply; 15+ messages in thread
From: Vladislav Shpilevoy @ 2020-11-11 21:48 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Hi! Thanks for the patch!

See 7 comments below.

On 03.11.2020 01:03, imeevma@tarantool.org wrote:
> After this patch, the persistent functions "box.schema.user.info" and
> "LUA" will have the same rights as the user who executed them.

1. It would be good to see more info what was wrong. Not only what
you changed, but also why. AFAIU, the issue was with the setuid bit
set without necessity.

> Fixes tarantool/security#1
> ---
> https://github.com/tarantool/security/issues/1
> https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access

2. Should there be a changelog record?

3. What about other versions? If this is a bug, it should be backported.
We plan to release 2.6.2, 2.5.3, 1.10.9. LUA() may be not actual for 1.10.

But I don't have a ready recipe how to backport a schema change. Assume
we will add upgrade_to_2_6_2(), which will do the same as upgrade_to_2_7_1().
Then if someone upgrades from 2.6.1, he will execute upgrade_to_2_6_2(), and
he does not need to call upgrade_to_2_7_1() (at least in its current form).

This is how I would try. I would move your fix to a new function with a
name like backport_upgrade_2_7_1_function_access(), or
backport_upgrade_security_1() (by repo and issue ID) which would check if
the functions have setuid set. If they do - remove it. Otherwise do nothing.
Then we call it in upgrade_to_1_10_9(), upgrade_to_2_5_3(), upgrade_to_2_6_2(),
and upgrade_to_2_7_1().

> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index add791cd7..671e441ca 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -971,6 +971,40 @@ local function upgrade_to_2_3_1()
>      create_session_settings_space()
>  end
>  
> +--------------------------------------------------------------------------------
> +-- Tarantool 2.7.1
> +--------------------------------------------------------------------------------
> +
> +local function upgrade_to_2_7_1()
> +    local _func = box.space[box.schema.FUNC_ID]
> +    local _priv = box.space[box.schema.PRIV_ID]

4. Why not _func = box.space._func and the same for _priv? Do you really need
to use the ids?

> +
> +    local datetime = os.date("%Y-%m-%d %H:%M:%S")
> +
> +    -- Re-create "box.schema.user.info" function.
> +    log.info('remove old function "box.schema.user.info"')
> +    _priv:delete({2, 'function', 1})
> +    _func:delete({1})
> +    log.info('create function "box.schema.user.info" with setuid')
> +    _func:replace({1, ADMIN, 'box.schema.user.info', 0, 'LUA', '', 'function',
> +                  {}, 'any', 'none', 'none', false, false, true, {'LUA'},
> +                  setmap({}), '', datetime, datetime})
> +    log.info('grant execute on function "box.schema.user.info" to public')
> +    _priv:replace{ADMIN, PUBLIC, 'function', 1, box.priv.X}
> +
> +    -- Re-create "LUA" function.
> +    log.info('remove old function "LUA"')
> +    _priv:delete({2, 'function', 65})
> +    _func:delete({65})

5. Can you not use the numeric constants? Can you search the needed
functions by name, and extract the ID from the result tuple?

> +    log.info('create function "LUA"')
> +    _func:replace({65, ADMIN, 'LUA', 0, 'LUA',
> +                   'function(code) return assert(loadstring(code))() end',
> +                   'function', {'string'}, 'any', 'none', 'none', false, false,
> +                   true, {'LUA', 'SQL'}, setmap({}), '', datetime, datetime})

6. Why did you in this replace align non-first lines by { + 1, and in the
previous replace exactly by {?

> +    log.info('grant execute on function "LUA" to public')
> +    _priv:replace{ADMIN, PUBLIC, 'function', 65, box.priv.X}
> +end
> diff --git a/test/box/access.result b/test/box/access.result
> index 20b1b8b35..92d6453d7 100644
> --- a/test/box/access.result
> +++ b/test/box/access.result
> @@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
>  sp:drop()
>  ---
>  ...
> +--
> +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
> +-- excess rights.
> +--
> +_ = box.schema.func.call("LUA", "return 1")
> +---
> +...
> +_ = box.schema.func.call("LUA", "return box.space._space:get(257)")

7. Why 257?

> +---
> +...
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +---
> +...
> +_ = box.schema.func.call("box.schema.user.info", 1)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
  2020-11-11 21:48 ` Vladislav Shpilevoy
@ 2020-12-03  9:54   ` Mergen Imeev
  2020-12-04 23:10     ` Vladislav Shpilevoy
  0 siblings, 1 reply; 15+ messages in thread
From: Mergen Imeev @ 2020-12-03  9:54 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi! Thank you for the review. Sorry for such late reply. New patch and my
answers below.

On Wed, Nov 11, 2020 at 10:48:54PM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> See 7 comments below.
> 
> On 03.11.2020 01:03, imeevma@tarantool.org wrote:
> > After this patch, the persistent functions "box.schema.user.info" and
> > "LUA" will have the same rights as the user who executed them.
> 
> 1. It would be good to see more info what was wrong. Not only what
> you changed, but also why. AFAIU, the issue was with the setuid bit
> set without necessity.
> 
Fixed.

> > Fixes tarantool/security#1
> > ---
> > https://github.com/tarantool/security/issues/1
> > https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access
> 
> 2. Should there be a changelog record?
> 
Added.

> 3. What about other versions? If this is a bug, it should be backported.
> We plan to release 2.6.2, 2.5.3, 1.10.9. LUA() may be not actual for 1.10.
> 
> But I don't have a ready recipe how to backport a schema change. Assume
> we will add upgrade_to_2_6_2(), which will do the same as upgrade_to_2_7_1().
> Then if someone upgrades from 2.6.1, he will execute upgrade_to_2_6_2(), and
> he does not need to call upgrade_to_2_7_1() (at least in its current form).
> 
> This is how I would try. I would move your fix to a new function with a
> name like backport_upgrade_2_7_1_function_access(), or
> backport_upgrade_security_1() (by repo and issue ID) which would check if
> the functions have setuid set. If they do - remove it. Otherwise do nothing.
> Then we call it in upgrade_to_1_10_9(), upgrade_to_2_5_3(), upgrade_to_2_6_2(),
> and upgrade_to_2_7_1().
> 
Thanks. I did as you suggested. Still, this patch only for 2.7. I will create
similar patches for other versions when this patch will be approved.

> > diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> > index add791cd7..671e441ca 100644
> > --- a/src/box/lua/upgrade.lua
> > +++ b/src/box/lua/upgrade.lua
> > @@ -971,6 +971,40 @@ local function upgrade_to_2_3_1()
> >      create_session_settings_space()
> >  end
> >  
> > +--------------------------------------------------------------------------------
> > +-- Tarantool 2.7.1
> > +--------------------------------------------------------------------------------
> > +
> > +local function upgrade_to_2_7_1()
> > +    local _func = box.space[box.schema.FUNC_ID]
> > +    local _priv = box.space[box.schema.PRIV_ID]
> 
> 4. Why not _func = box.space._func and the same for _priv? Do you really need
> to use the ids?
> 
Fixed.

> > +
> > +    local datetime = os.date("%Y-%m-%d %H:%M:%S")
> > +
> > +    -- Re-create "box.schema.user.info" function.
> > +    log.info('remove old function "box.schema.user.info"')
> > +    _priv:delete({2, 'function', 1})
> > +    _func:delete({1})
> > +    log.info('create function "box.schema.user.info" with setuid')
> > +    _func:replace({1, ADMIN, 'box.schema.user.info', 0, 'LUA', '', 'function',
> > +                  {}, 'any', 'none', 'none', false, false, true, {'LUA'},
> > +                  setmap({}), '', datetime, datetime})
> > +    log.info('grant execute on function "box.schema.user.info" to public')
> > +    _priv:replace{ADMIN, PUBLIC, 'function', 1, box.priv.X}
> > +
> > +    -- Re-create "LUA" function.
> > +    log.info('remove old function "LUA"')
> > +    _priv:delete({2, 'function', 65})
> > +    _func:delete({65})
> 
> 5. Can you not use the numeric constants? Can you search the needed
> functions by name, and extract the ID from the result tuple?
> 
Rewrote this part.

> > +    log.info('create function "LUA"')
> > +    _func:replace({65, ADMIN, 'LUA', 0, 'LUA',
> > +                   'function(code) return assert(loadstring(code))() end',
> > +                   'function', {'string'}, 'any', 'none', 'none', false, false,
> > +                   true, {'LUA', 'SQL'}, setmap({}), '', datetime, datetime})
> 
> 6. Why did you in this replace align non-first lines by { + 1, and in the
> previous replace exactly by {?
> 
Removed in rewroted part.

> > +    log.info('grant execute on function "LUA" to public')
> > +    _priv:replace{ADMIN, PUBLIC, 'function', 65, box.priv.X}
> > +end
> > diff --git a/test/box/access.result b/test/box/access.result
> > index 20b1b8b35..92d6453d7 100644
> > --- a/test/box/access.result
> > +++ b/test/box/access.result
> > @@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
> >  sp:drop()
> >  ---
> >  ...
> > +--
> > +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
> > +-- excess rights.
> > +--
> > +_ = box.schema.func.call("LUA", "return 1")
> > +---
> > +...
> > +_ = box.schema.func.call("LUA", "return box.space._space:get(257)")
> 
> 7. Why 257?
> 
Changed get() to count().

> > +---
> > +...
> > +_ = box.schema.func.call("box.schema.user.info", 0)
> > +---
> > +...
> > +_ = box.schema.func.call("box.schema.user.info", 1)

New patch:

From 785013ce70c43f68678aa3a558248979b6b7c6e1 Mon Sep 17 00:00:00 2001
Message-Id: <785013ce70c43f68678aa3a558248979b6b7c6e1.1606988615.git.imeevma@gmail.com>
From: Mergen Imeev <imeevma@gmail.com>
Date: Mon, 2 Nov 2020 13:21:58 +0300
Subject: [PATCH v2 1/1] box: remove unnecessary rights from peristent
 functions

After this patch, the persistent functions "box.schema.user.info" and
"LUA" will have the same rights as the user who executed them.

The problem was that setuid was unnecessarily set. Because of this,
these functions had the same rights as the user who created them.
However, they must have the same rights as the user who used them.

Fixes tarantool/security#1
---
https://github.com/tarantool/security/issues/1
https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access

@ChangeLog
 - The functions "LUA" and "box.schema.user.info" now have the
   same rights as the user who executes them. (gh-security-1).

 src/box/bootstrap.snap       | Bin 5976 -> 5990 bytes
 src/box/lua/upgrade.lua      |  31 ++++++++++++++++++++++++++++++
 test/box-py/bootstrap.result |   4 ++--
 test/box/access.result       |  36 +++++++++++++++++++++++++++++++++++
 test/box/access.test.lua     |  15 +++++++++++++++
 5 files changed, 84 insertions(+), 2 deletions(-)

diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index add791cd7..b2475b0f6 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -971,6 +971,36 @@ local function upgrade_to_2_3_1()
     create_session_settings_space()
 end
 
+--------------------------------------------------------------------------------
+-- Tarantool 2.7.1
+--------------------------------------------------------------------------------
+local function backport_upgrade_2_7_1_function_access()
+    local _func = box.space._func
+    local _priv = box.space._priv
+    local datetime = os.date("%Y-%m-%d %H:%M:%S")
+    local funcs_to_change = {'LUA', 'box.schema.user.info'}
+    for _, name in pairs(funcs_to_change) do
+        local func = _func.index['name']:get(name)
+        -- Change setuid of function function if it is not 0.
+        if func ~= nil and func.setuid ~= 0 then
+            local id = func.id
+            log.info('remove old function "'..name..'"')
+            _priv:delete({2, 'function', id})
+            _func:delete({id})
+            log.info('create function "'..name..'" with unset setuid')
+            local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime},
+                                          {'=', 19, datetime}})
+            _func:replace(new_func)
+            log.info('grant execute on function "'..name..'" to public')
+            _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X}
+        end
+    end
+end
+
+local function upgrade_to_2_7_1()
+    backport_upgrade_2_7_1_function_access()
+end
+
 --------------------------------------------------------------------------------
 
 local function get_version()
@@ -1007,6 +1037,7 @@ local function upgrade(options)
         {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
         {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
         {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
+        {version = mkversion(2, 7, 1), func = upgrade_to_2_7_1, auto = true},
     }
 
     for _, handler in ipairs(handlers) do
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 0876e77a6..ed7accea3 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -4,7 +4,7 @@ box.internal.bootstrap()
 box.space._schema:select{}
 ---
 - - ['max_id', 511]
-  - ['version', 2, 3, 1]
+  - ['version', 2, 7, 1]
 ...
 box.space._cluster:select{}
 ---
@@ -167,7 +167,7 @@ box.space._user:select{}
 ...
 for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end
 ---
-- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none',
+- - [1, 1, 'box.schema.user.info', 0, 'LUA', '', 'function', [], 'any', 'none', 'none',
     false, false, true, ['LUA'], {}, '', '', '']
 ...
 box.space._priv:select{}
diff --git a/test/box/access.result b/test/box/access.result
index 20b1b8b35..27e636122 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
 ---
 ...
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+...
+session.su('guest')
+---
+...
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+---
+- error: Read access to space '_space' is denied for user 'guest'
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+- error: User '1' is not found
+...
+session.su('admin')
+---
+...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 3e083a383..a62f87ad8 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -824,3 +824,18 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
+
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('guest')
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('admin')
-- 
2.25.1

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
  2020-12-03  9:54   ` Mergen Imeev
@ 2020-12-04 23:10     ` Vladislav Shpilevoy
  2020-12-16 20:23       ` Mergen Imeev
  0 siblings, 1 reply; 15+ messages in thread
From: Vladislav Shpilevoy @ 2020-12-04 23:10 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Hi! Thanks for the patch!

It looks good except for one comment below. Please, proceed to
making it work on all versions, like we discussed.

> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index add791cd7..b2475b0f6 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -971,6 +971,36 @@ local function upgrade_to_2_3_1()
>      create_session_settings_space()
>  end
>  
> +--------------------------------------------------------------------------------
> +-- Tarantool 2.7.1
> +--------------------------------------------------------------------------------
> +local function backport_upgrade_2_7_1_function_access()
> +    local _func = box.space._func
> +    local _priv = box.space._priv
> +    local datetime = os.date("%Y-%m-%d %H:%M:%S")
> +    local funcs_to_change = {'LUA', 'box.schema.user.info'}
> +    for _, name in pairs(funcs_to_change) do
> +        local func = _func.index['name']:get(name)
> +        -- Change setuid of function function if it is not 0.

"function function"? Tbh, the entire comment looks unneeded. It
just literally narrates the condition check. If you want to have
a comment here, better explain what is wrong with having setuid
set. Or nothing.

> +        if func ~= nil and func.setuid ~= 0 then
> +            local id = func.id
> +            log.info('remove old function "'..name..'"')
> +            _priv:delete({2, 'function', id})
> +            _func:delete({id})
> +            log.info('create function "'..name..'" with unset setuid')
> +            local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime},
> +                                          {'=', 19, datetime}})
> +            _func:replace(new_func)
> +            log.info('grant execute on function "'..name..'" to public')
> +            _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X}
> +        end
> +    end
> +end
> +
> +local function upgrade_to_2_7_1()
> +    backport_upgrade_2_7_1_function_access()
> +end

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
  2020-12-04 23:10     ` Vladislav Shpilevoy
@ 2020-12-16 20:23       ` Mergen Imeev
  2020-12-20 14:05         ` Vladislav Shpilevoy
  0 siblings, 1 reply; 15+ messages in thread
From: Mergen Imeev @ 2020-12-16 20:23 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi! Thank you for the review. My answer and patches for 2.7, 2.6 and 2.5
below.

On Sat, Dec 05, 2020 at 12:10:38AM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> It looks good except for one comment below. Please, proceed to
> making it work on all versions, like we discussed.
> 
> > diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> > index add791cd7..b2475b0f6 100644
> > --- a/src/box/lua/upgrade.lua
> > +++ b/src/box/lua/upgrade.lua
> > @@ -971,6 +971,36 @@ local function upgrade_to_2_3_1()
> >      create_session_settings_space()
> >  end
> >  
> > +--------------------------------------------------------------------------------
> > +-- Tarantool 2.7.1
> > +--------------------------------------------------------------------------------
> > +local function backport_upgrade_2_7_1_function_access()
> > +    local _func = box.space._func
> > +    local _priv = box.space._priv
> > +    local datetime = os.date("%Y-%m-%d %H:%M:%S")
> > +    local funcs_to_change = {'LUA', 'box.schema.user.info'}
> > +    for _, name in pairs(funcs_to_change) do
> > +        local func = _func.index['name']:get(name)
> > +        -- Change setuid of function function if it is not 0.
> 
> "function function"? Tbh, the entire comment looks unneeded. It
> just literally narrates the condition check. If you want to have
> a comment here, better explain what is wrong with having setuid
> set. Or nothing.
> 
Removed.

> > +        if func ~= nil and func.setuid ~= 0 then
> > +            local id = func.id
> > +            log.info('remove old function "'..name..'"')
> > +            _priv:delete({2, 'function', id})
> > +            _func:delete({id})
> > +            log.info('create function "'..name..'" with unset setuid')
> > +            local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime},
> > +                                          {'=', 19, datetime}})
> > +            _func:replace(new_func)
> > +            log.info('grant execute on function "'..name..'" to public')
> > +            _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X}
> > +        end
> > +    end
> > +end
> > +
> > +local function upgrade_to_2_7_1()
> > +    backport_upgrade_2_7_1_function_access()
> > +end


Patch for 2.7:

From 8c15cec7f73f0ba733b22e9e03e090c9e6f672eb Mon Sep 17 00:00:00 2001
Message-Id: <8c15cec7f73f0ba733b22e9e03e090c9e6f672eb.1608147417.git.imeevma@gmail.com>
From: Mergen Imeev <imeevma@gmail.com>
Date: Mon, 2 Nov 2020 13:21:58 +0300
Subject: [PATCH v1 1/1] box: remove unnecessary rights from peristent
 functions

After this patch, the persistent functions "box.schema.user.info" and
"LUA" will have the same rights as the user who executed them.

The problem was that setuid was unnecessarily set. Because of this,
these functions had the same rights as the user who created them.
However, they must have the same rights as the user who used them.

Fixes tarantool/security#1
---

https://github.com/tarantool/security/issues/1
https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access

@ChangeLog
 - The functions "LUA" and "box.schema.user.info" now have the
   same rights as the user who executes them. (gh-security-1).

 src/box/bootstrap.snap       | Bin 5976 -> 5991 bytes
 src/box/lua/upgrade.lua      |  30 +++++++++++++++++++++++++++++
 test/box-py/bootstrap.result |   4 ++--
 test/box/access.result       |  36 +++++++++++++++++++++++++++++++++++
 test/box/access.test.lua     |  15 +++++++++++++++
 5 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index a86a0d410..09cd015f0 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -971,6 +971,35 @@ local function upgrade_to_2_3_1()
     create_session_settings_space()
 end
 
+--------------------------------------------------------------------------------
+-- Tarantool 2.7.1
+--------------------------------------------------------------------------------
+local function backport_upgrade_2_7_1_function_access()
+    local _func = box.space._func
+    local _priv = box.space._priv
+    local datetime = os.date("%Y-%m-%d %H:%M:%S")
+    local funcs_to_change = {'LUA', 'box.schema.user.info'}
+    for _, name in pairs(funcs_to_change) do
+        local func = _func.index['name']:get(name)
+        if func ~= nil and func.setuid ~= 0 then
+            local id = func.id
+            log.info('remove old function "'..name..'"')
+            _priv:delete({2, 'function', id})
+            _func:delete({id})
+            log.info('create function "'..name..'" with unset setuid')
+            local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime},
+                                          {'=', 19, datetime}})
+            _func:replace(new_func)
+            log.info('grant execute on function "'..name..'" to public')
+            _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X}
+        end
+    end
+end
+
+local function upgrade_to_2_7_1()
+    backport_upgrade_2_7_1_function_access()
+end
+
 --------------------------------------------------------------------------------
 
 local handlers = {
@@ -985,6 +1014,7 @@ local handlers = {
     {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
     {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
     {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
+    {version = mkversion(2, 7, 1), func = upgrade_to_2_7_1, auto = true},
 }
 
 -- Schema version of the snapshot.
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 0876e77a6..ed7accea3 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -4,7 +4,7 @@ box.internal.bootstrap()
 box.space._schema:select{}
 ---
 - - ['max_id', 511]
-  - ['version', 2, 3, 1]
+  - ['version', 2, 7, 1]
 ...
 box.space._cluster:select{}
 ---
@@ -167,7 +167,7 @@ box.space._user:select{}
 ...
 for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end
 ---
-- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none',
+- - [1, 1, 'box.schema.user.info', 0, 'LUA', '', 'function', [], 'any', 'none', 'none',
     false, false, true, ['LUA'], {}, '', '', '']
 ...
 box.space._priv:select{}
diff --git a/test/box/access.result b/test/box/access.result
index 20b1b8b35..27e636122 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
 ---
 ...
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+...
+session.su('guest')
+---
+...
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+---
+- error: Read access to space '_space' is denied for user 'guest'
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+- error: User '1' is not found
+...
+session.su('admin')
+---
+...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 3e083a383..a62f87ad8 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -824,3 +824,18 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
+
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('guest')
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('admin')
-- 
2.25.1



Patch for 2.6:


From db5054f8781e0f438e7a29beb8e680afaffbb106 Mon Sep 17 00:00:00 2001
Message-Id: <db5054f8781e0f438e7a29beb8e680afaffbb106.1608148610.git.imeevma@gmail.com>
From: Mergen Imeev <imeevma@gmail.com>
Date: Mon, 2 Nov 2020 13:21:58 +0300
Subject: [PATCH v1 1/1] box: remove unnecessary rights from peristent
 functions

After this patch, the persistent functions "box.schema.user.info" and
"LUA" will have the same rights as the user who executed them.

The problem was that setuid was unnecessarily set. Because of this,
these functions had the same rights as the user who created them.
However, they must have the same rights as the user who used them.

Part of tarantool/security#1
---
 src/box/bootstrap.snap       | Bin 5976 -> 5997 bytes
 src/box/lua/upgrade.lua      |  30 +++++++++++++++++++++++++++++
 test/box-py/bootstrap.result |   4 ++--
 test/box/access.result       |  36 +++++++++++++++++++++++++++++++++++
 test/box/access.test.lua     |  15 +++++++++++++++
 5 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index add791cd7..be1a0d47d 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -971,6 +971,35 @@ local function upgrade_to_2_3_1()
     create_session_settings_space()
 end
 
+--------------------------------------------------------------------------------
+-- Tarantool 2.6.2
+--------------------------------------------------------------------------------
+local function backport_upgrade_2_7_1_function_access()
+    local _func = box.space._func
+    local _priv = box.space._priv
+    local datetime = os.date("%Y-%m-%d %H:%M:%S")
+    local funcs_to_change = {'LUA', 'box.schema.user.info'}
+    for _, name in pairs(funcs_to_change) do
+        local func = _func.index['name']:get(name)
+        if func ~= nil and func.setuid ~= 0 then
+            local id = func.id
+            log.info('remove old function "'..name..'"')
+            _priv:delete({2, 'function', id})
+            _func:delete({id})
+            log.info('create function "'..name..'" with unset setuid')
+            local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime},
+                                          {'=', 19, datetime}})
+            _func:replace(new_func)
+            log.info('grant execute on function "'..name..'" to public')
+            _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X}
+        end
+    end
+end
+
+local function upgrade_to_2_6_2()
+    backport_upgrade_2_7_1_function_access()
+end
+
 --------------------------------------------------------------------------------
 
 local function get_version()
@@ -1007,6 +1036,7 @@ local function upgrade(options)
         {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
         {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
         {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
+        {version = mkversion(2, 6, 2), func = upgrade_to_2_6_2, auto = true},
     }
 
     for _, handler in ipairs(handlers) do
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 0876e77a6..a371bb369 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -4,7 +4,7 @@ box.internal.bootstrap()
 box.space._schema:select{}
 ---
 - - ['max_id', 511]
-  - ['version', 2, 3, 1]
+  - ['version', 2, 6, 2]
 ...
 box.space._cluster:select{}
 ---
@@ -167,7 +167,7 @@ box.space._user:select{}
 ...
 for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end
 ---
-- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none',
+- - [1, 1, 'box.schema.user.info', 0, 'LUA', '', 'function', [], 'any', 'none', 'none',
     false, false, true, ['LUA'], {}, '', '', '']
 ...
 box.space._priv:select{}
diff --git a/test/box/access.result b/test/box/access.result
index 20b1b8b35..27e636122 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
 ---
 ...
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+...
+session.su('guest')
+---
+...
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+---
+- error: Read access to space '_space' is denied for user 'guest'
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+- error: User '1' is not found
+...
+session.su('admin')
+---
+...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 3e083a383..a62f87ad8 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -824,3 +824,18 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
+
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('guest')
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('admin')
-- 
2.25.1



Patch for 2.5:


From e3c825d301bc699e2020cac861962aeb5b0758f1 Mon Sep 17 00:00:00 2001
Message-Id: <e3c825d301bc699e2020cac861962aeb5b0758f1.1608149922.git.imeevma@gmail.com>
From: Mergen Imeev <imeevma@gmail.com>
Date: Mon, 2 Nov 2020 13:21:58 +0300
Subject: [PATCH v1 1/1] box: remove unnecessary rights from peristent
 functions

After this patch, the persistent functions "box.schema.user.info" and
"LUA" will have the same rights as the user who executed them.

The problem was that setuid was unnecessarily set. Because of this,
these functions had the same rights as the user who created them.
However, they must have the same rights as the user who used them.

Part of tarantool/security#1
---
 src/box/bootstrap.snap       | Bin 5976 -> 5975 bytes
 src/box/lua/upgrade.lua      |  30 +++++++++++++++++++++++++++++
 test/box-py/bootstrap.result |   4 ++--
 test/box/access.result       |  36 +++++++++++++++++++++++++++++++++++
 test/box/access.test.lua     |  15 +++++++++++++++
 5 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index add791cd7..e5ddd513f 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -971,6 +971,35 @@ local function upgrade_to_2_3_1()
     create_session_settings_space()
 end
 
+--------------------------------------------------------------------------------
+-- Tarantool 2.5.3
+--------------------------------------------------------------------------------
+local function backport_upgrade_2_7_1_function_access()
+    local _func = box.space._func
+    local _priv = box.space._priv
+    local datetime = os.date("%Y-%m-%d %H:%M:%S")
+    local funcs_to_change = {'LUA', 'box.schema.user.info'}
+    for _, name in pairs(funcs_to_change) do
+        local func = _func.index['name']:get(name)
+        if func ~= nil and func.setuid ~= 0 then
+            local id = func.id
+            log.info('remove old function "'..name..'"')
+            _priv:delete({2, 'function', id})
+            _func:delete({id})
+            log.info('create function "'..name..'" with unset setuid')
+            local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime},
+                                          {'=', 19, datetime}})
+            _func:replace(new_func)
+            log.info('grant execute on function "'..name..'" to public')
+            _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X}
+        end
+    end
+end
+
+local function upgrade_to_2_5_3()
+    backport_upgrade_2_7_1_function_access()
+end
+
 --------------------------------------------------------------------------------
 
 local function get_version()
@@ -1007,6 +1036,7 @@ local function upgrade(options)
         {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
         {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
         {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
+        {version = mkversion(2, 5, 3), func = upgrade_to_2_5_3, auto = true},
     }
 
     for _, handler in ipairs(handlers) do
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 0876e77a6..9ebccc56d 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -4,7 +4,7 @@ box.internal.bootstrap()
 box.space._schema:select{}
 ---
 - - ['max_id', 511]
-  - ['version', 2, 3, 1]
+  - ['version', 2, 5, 3]
 ...
 box.space._cluster:select{}
 ---
@@ -167,7 +167,7 @@ box.space._user:select{}
 ...
 for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end
 ---
-- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none',
+- - [1, 1, 'box.schema.user.info', 0, 'LUA', '', 'function', [], 'any', 'none', 'none',
     false, false, true, ['LUA'], {}, '', '', '']
 ...
 box.space._priv:select{}
diff --git a/test/box/access.result b/test/box/access.result
index 20b1b8b35..27e636122 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
 ---
 ...
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+...
+session.su('guest')
+---
+...
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+---
+- error: Read access to space '_space' is denied for user 'guest'
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+- error: User '1' is not found
+...
+session.su('admin')
+---
+...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 3e083a383..a62f87ad8 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -824,3 +824,18 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
+
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('guest')
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('admin')
-- 
2.25.1

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
  2020-12-16 20:23       ` Mergen Imeev
@ 2020-12-20 14:05         ` Vladislav Shpilevoy
  0 siblings, 0 replies; 15+ messages in thread
From: Vladislav Shpilevoy @ 2020-12-20 14:05 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Hi! Thanks for the patch!

LGTM.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
  2020-12-21 10:51 imeevma
  2020-12-21 19:23 ` Sergey Ostanevich
@ 2020-12-23 12:58 ` Kirill Yukhin
  1 sibling, 0 replies; 15+ messages in thread
From: Kirill Yukhin @ 2020-12-23 12:58 UTC (permalink / raw)
  To: imeevma; +Cc: s.ostanevich, tarantool-patches

Hello,

On 21 Dec 13:51, Mergen Imeev via Tarantool-patches wrote:
> After this patch, the persistent functions "box.schema.user.info" and
> "LUA" will have the same rights as the user who executed them.
> 
> The problem was that setuid was unnecessarily set. Because of this,
> these functions had the same rights as the user who created them.
> However, they must have the same rights as the user who used them.
> 
> Fixes tarantool/security#1

I've checked your patch into 2.5, 2.6 and masterr.

--
Regards, Kirill Yukhin

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
  2020-12-22 17:44   ` Mergen Imeev
@ 2020-12-23  9:44     ` Sergey Ostanevich
  0 siblings, 0 replies; 15+ messages in thread
From: Sergey Ostanevich @ 2020-12-23  9:44 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Hi!

LGTM.

Sergos

> On 22 Dec 2020, at 20:44, Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org> wrote:
> 
> Hi! Thank you for the review! My answer and new patch below.
> 
> On Mon, Dec 21, 2020 at 10:23:01PM +0300, Sergey Ostanevich wrote:
>> Hi!
>> 
>> Thanks for the patch!
>> 
>> I wonder why we should have this strange naming for the function of
>> backport 2_7_1 from 2_7_1 itself. Perhaps just the ‘function_access’
>> is enough?
> Fixed.
> 
>> 
>> Otherwise LGTM.
>> 
>> Sergos
>> 
>> 
>>> On 21 Dec 2020, at 13:51, Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org> wrote:
>>> 
>>> After this patch, the persistent functions "box.schema.user.info" and
>>> "LUA" will have the same rights as the user who executed them.
>>> 
>>> The problem was that setuid was unnecessarily set. Because of this,
>>> these functions had the same rights as the user who created them.
>>> However, they must have the same rights as the user who used them.
>>> 
>>> Fixes tarantool/security#1
>>> ---
>>> https://github.com/tarantool/security/issues/1
>>> https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access
>>> 
>>> @ChangeLog
>>> - The functions "LUA" and "box.schema.user.info" now have the
>>>  same rights as the user who executes them. (gh-security-1).
>>> 
>>> 
>>> src/box/bootstrap.snap       | Bin 5976 -> 5991 bytes
>>> src/box/lua/upgrade.lua      |  30 +++++++++++++++++++++++++++++
>>> test/box-py/bootstrap.result |   4 ++--
>>> test/box/access.result       |  36 +++++++++++++++++++++++++++++++++++
>>> test/box/access.test.lua     |  15 +++++++++++++++
>>> 5 files changed, 83 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
>>> index 8bd4f7ce24216a8bcced6aa97c20c83eb7a02c77..9e7a4e3291accfc387b315f85be9b01f443048fd 100644
>>> 
>>> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
>>> index a86a0d410..09cd015f0 100644
>>> --- a/src/box/lua/upgrade.lua
>>> +++ b/src/box/lua/upgrade.lua
>>> @@ -971,6 +971,35 @@ local function upgrade_to_2_3_1()
>>>    create_session_settings_space()
>>> end
>>> 
>>> +--------------------------------------------------------------------------------
>>> +-- Tarantool 2.7.1
>>> +--------------------------------------------------------------------------------
>>> +local function backport_upgrade_2_7_1_function_access()
>>> +    local _func = box.space._func
>>> +    local _priv = box.space._priv
>>> +    local datetime = os.date("%Y-%m-%d %H:%M:%S")
>>> +    local funcs_to_change = {'LUA', 'box.schema.user.info'}
>>> +    for _, name in pairs(funcs_to_change) do
>>> +        local func = _func.index['name']:get(name)
>>> +        if func ~= nil and func.setuid ~= 0 then
>>> +            local id = func.id
>>> +            log.info('remove old function "'..name..'"')
>>> +            _priv:delete({2, 'function', id})
>>> +            _func:delete({id})
>>> +            log.info('create function "'..name..'" with unset setuid')
>>> +            local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime},
>>> +                                          {'=', 19, datetime}})
>>> +            _func:replace(new_func)
>>> +            log.info('grant execute on function "'..name..'" to public')
>>> +            _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X}
>>> +        end
>>> +    end
>>> +end
>>> +
>>> +local function upgrade_to_2_7_1()
>>> +    backport_upgrade_2_7_1_function_access()
>>> +end
>>> +
>>> --------------------------------------------------------------------------------
>>> 
>>> local handlers = {
>>> @@ -985,6 +1014,7 @@ local handlers = {
>>>    {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
>>>    {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
>>>    {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
>>> +    {version = mkversion(2, 7, 1), func = upgrade_to_2_7_1, auto = true},
>>> }
>>> 
>>> -- Schema version of the snapshot.
>>> diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
>>> index 0876e77a6..ed7accea3 100644
>>> --- a/test/box-py/bootstrap.result
>>> +++ b/test/box-py/bootstrap.result
>>> @@ -4,7 +4,7 @@ box.internal.bootstrap()
>>> box.space._schema:select{}
>>> ---
>>> - - ['max_id', 511]
>>> -  - ['version', 2, 3, 1]
>>> +  - ['version', 2, 7, 1]
>>> ...
>>> box.space._cluster:select{}
>>> ---
>>> @@ -167,7 +167,7 @@ box.space._user:select{}
>>> ...
>>> for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end
>>> ---
>>> -- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none',
>>> +- - [1, 1, 'box.schema.user.info', 0, 'LUA', '', 'function', [], 'any', 'none', 'none',
>>>    false, false, true, ['LUA'], {}, '', '', '']
>>> ...
>>> box.space._priv:select{}
>>> diff --git a/test/box/access.result b/test/box/access.result
>>> index 20b1b8b35..27e636122 100644
>>> --- a/test/box/access.result
>>> +++ b/test/box/access.result
>>> @@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
>>> sp:drop()
>>> ---
>>> ...
>>> +--
>>> +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
>>> +-- excess rights.
>>> +--
>>> +_ = box.schema.func.call("LUA", "return 1")
>>> +---
>>> +...
>>> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
>>> +---
>>> +...
>>> +_ = box.schema.func.call("box.schema.user.info", 0)
>>> +---
>>> +...
>>> +_ = box.schema.func.call("box.schema.user.info", 1)
>>> +---
>>> +...
>>> +session.su('guest')
>>> +---
>>> +...
>>> +_ = box.schema.func.call("LUA", "return 1")
>>> +---
>>> +...
>>> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
>>> +---
>>> +- error: Read access to space '_space' is denied for user 'guest'
>>> +...
>>> +_ = box.schema.func.call("box.schema.user.info", 0)
>>> +---
>>> +...
>>> +_ = box.schema.func.call("box.schema.user.info", 1)
>>> +---
>>> +- error: User '1' is not found
>>> +...
>>> +session.su('admin')
>>> +---
>>> +...
>>> diff --git a/test/box/access.test.lua b/test/box/access.test.lua
>>> index 3e083a383..a62f87ad8 100644
>>> --- a/test/box/access.test.lua
>>> +++ b/test/box/access.test.lua
>>> @@ -824,3 +824,18 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
>>> box.schema.user.revoke('guest', 'read,write,execute', 'universe')
>>> box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
>>> sp:drop()
>>> +
>>> +--
>>> +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
>>> +-- excess rights.
>>> +--
>>> +_ = box.schema.func.call("LUA", "return 1")
>>> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
>>> +_ = box.schema.func.call("box.schema.user.info", 0)
>>> +_ = box.schema.func.call("box.schema.user.info", 1)
>>> +session.su('guest')
>>> +_ = box.schema.func.call("LUA", "return 1")
>>> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
>>> +_ = box.schema.func.call("box.schema.user.info", 0)
>>> +_ = box.schema.func.call("box.schema.user.info", 1)
>>> +session.su('admin')
>>> -- 
>>> 2.25.1
>>> 
>> 
> 
> 
> From 21efff563826e47ea049aff2c7215b5bac5883e6 Mon Sep 17 00:00:00 2001
> Message-Id: <21efff563826e47ea049aff2c7215b5bac5883e6.1608658852.git.imeevma@gmail.com>
> From: Mergen Imeev <imeevma@gmail.com>
> Date: Mon, 2 Nov 2020 13:21:58 +0300
> Subject: [PATCH v1 1/1] box: remove unnecessary rights from peristent
> functions
> 
> After this patch, the persistent functions "box.schema.user.info" and
> "LUA" will have the same rights as the user who executed them.
> 
> The problem was that setuid was unnecessarily set. Because of this,
> these functions had the same rights as the user who created them.
> However, they must have the same rights as the user who used them.
> 
> Fixes tarantool/security#1
> ---
> https://github.com/tarantool/security/issues/1
> https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access
> 
> @ChangeLog
> - The functions "LUA" and "box.schema.user.info" now have the
>  same rights as the user who executes them. (gh-security-1).
> 
> 
> src/box/bootstrap.snap       | Bin 5976 -> 5991 bytes
> src/box/lua/upgrade.lua      |  30 +++++++++++++++++++++++++++++
> test/box-py/bootstrap.result |   4 ++--
> test/box/access.result       |  36 +++++++++++++++++++++++++++++++++++
> test/box/access.test.lua     |  15 +++++++++++++++
> 5 files changed, 83 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
> index 8bd4f7ce24216a8bcced6aa97c20c83eb7a02c77..c4a70297aad138d426a24ee4447af485e3597536 100644
> 
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index a86a0d410..6fba260bd 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -971,6 +971,35 @@ local function upgrade_to_2_3_1()
>     create_session_settings_space()
> end
> 
> +--------------------------------------------------------------------------------
> +-- Tarantool 2.7.1
> +--------------------------------------------------------------------------------
> +local function function_access()
> +    local _func = box.space._func
> +    local _priv = box.space._priv
> +    local datetime = os.date("%Y-%m-%d %H:%M:%S")
> +    local funcs_to_change = {'LUA', 'box.schema.user.info'}
> +    for _, name in pairs(funcs_to_change) do
> +        local func = _func.index['name']:get(name)
> +        if func ~= nil and func.setuid ~= 0 then
> +            local id = func.id
> +            log.info('remove old function "'..name..'"')
> +            _priv:delete({2, 'function', id})
> +            _func:delete({id})
> +            log.info('create function "'..name..'" with unset setuid')
> +            local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime},
> +                                          {'=', 19, datetime}})
> +            _func:replace(new_func)
> +            log.info('grant execute on function "'..name..'" to public')
> +            _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X}
> +        end
> +    end
> +end
> +
> +local function upgrade_to_2_7_1()
> +    function_access()
> +end
> +
> --------------------------------------------------------------------------------
> 
> local handlers = {
> @@ -985,6 +1014,7 @@ local handlers = {
>     {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
>     {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
>     {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
> +    {version = mkversion(2, 7, 1), func = upgrade_to_2_7_1, auto = true},
> }
> 
> -- Schema version of the snapshot.
> diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
> index 0876e77a6..ed7accea3 100644
> --- a/test/box-py/bootstrap.result
> +++ b/test/box-py/bootstrap.result
> @@ -4,7 +4,7 @@ box.internal.bootstrap()
> box.space._schema:select{}
> ---
> - - ['max_id', 511]
> -  - ['version', 2, 3, 1]
> +  - ['version', 2, 7, 1]
> ...
> box.space._cluster:select{}
> ---
> @@ -167,7 +167,7 @@ box.space._user:select{}
> ...
> for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end
> ---
> -- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none',
> +- - [1, 1, 'box.schema.user.info', 0, 'LUA', '', 'function', [], 'any', 'none', 'none',
>     false, false, true, ['LUA'], {}, '', '', '']
> ...
> box.space._priv:select{}
> diff --git a/test/box/access.result b/test/box/access.result
> index 20b1b8b35..27e636122 100644
> --- a/test/box/access.result
> +++ b/test/box/access.result
> @@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
> sp:drop()
> ---
> ...
> +--
> +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
> +-- excess rights.
> +--
> +_ = box.schema.func.call("LUA", "return 1")
> +---
> +...
> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> +---
> +...
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +---
> +...
> +_ = box.schema.func.call("box.schema.user.info", 1)
> +---
> +...
> +session.su('guest')
> +---
> +...
> +_ = box.schema.func.call("LUA", "return 1")
> +---
> +...
> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> +---
> +- error: Read access to space '_space' is denied for user 'guest'
> +...
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +---
> +...
> +_ = box.schema.func.call("box.schema.user.info", 1)
> +---
> +- error: User '1' is not found
> +...
> +session.su('admin')
> +---
> +...
> diff --git a/test/box/access.test.lua b/test/box/access.test.lua
> index 3e083a383..a62f87ad8 100644
> --- a/test/box/access.test.lua
> +++ b/test/box/access.test.lua
> @@ -824,3 +824,18 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
> box.schema.user.revoke('guest', 'read,write,execute', 'universe')
> box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
> sp:drop()
> +
> +--
> +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
> +-- excess rights.
> +--
> +_ = box.schema.func.call("LUA", "return 1")
> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +_ = box.schema.func.call("box.schema.user.info", 1)
> +session.su('guest')
> +_ = box.schema.func.call("LUA", "return 1")
> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +_ = box.schema.func.call("box.schema.user.info", 1)
> +session.su('admin')
> -- 
> 2.25.1

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
  2020-12-21 19:23 ` Sergey Ostanevich
@ 2020-12-22 17:44   ` Mergen Imeev
  2020-12-23  9:44     ` Sergey Ostanevich
  0 siblings, 1 reply; 15+ messages in thread
From: Mergen Imeev @ 2020-12-22 17:44 UTC (permalink / raw)
  To: Sergey Ostanevich; +Cc: tarantool-patches

Hi! Thank you for the review! My answer and new patch below.

On Mon, Dec 21, 2020 at 10:23:01PM +0300, Sergey Ostanevich wrote:
> Hi!
> 
> Thanks for the patch!
> 
> I wonder why we should have this strange naming for the function of
> backport 2_7_1 from 2_7_1 itself. Perhaps just the ‘function_access’
> is enough?
Fixed.

> 
> Otherwise LGTM.
> 
> Sergos
> 
> 
> > On 21 Dec 2020, at 13:51, Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org> wrote:
> > 
> > After this patch, the persistent functions "box.schema.user.info" and
> > "LUA" will have the same rights as the user who executed them.
> > 
> > The problem was that setuid was unnecessarily set. Because of this,
> > these functions had the same rights as the user who created them.
> > However, they must have the same rights as the user who used them.
> > 
> > Fixes tarantool/security#1
> > ---
> > https://github.com/tarantool/security/issues/1
> > https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access
> > 
> > @ChangeLog
> > - The functions "LUA" and "box.schema.user.info" now have the
> >   same rights as the user who executes them. (gh-security-1).
> > 
> > 
> > src/box/bootstrap.snap       | Bin 5976 -> 5991 bytes
> > src/box/lua/upgrade.lua      |  30 +++++++++++++++++++++++++++++
> > test/box-py/bootstrap.result |   4 ++--
> > test/box/access.result       |  36 +++++++++++++++++++++++++++++++++++
> > test/box/access.test.lua     |  15 +++++++++++++++
> > 5 files changed, 83 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
> > index 8bd4f7ce24216a8bcced6aa97c20c83eb7a02c77..9e7a4e3291accfc387b315f85be9b01f443048fd 100644
> > 
> > diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> > index a86a0d410..09cd015f0 100644
> > --- a/src/box/lua/upgrade.lua
> > +++ b/src/box/lua/upgrade.lua
> > @@ -971,6 +971,35 @@ local function upgrade_to_2_3_1()
> >     create_session_settings_space()
> > end
> > 
> > +--------------------------------------------------------------------------------
> > +-- Tarantool 2.7.1
> > +--------------------------------------------------------------------------------
> > +local function backport_upgrade_2_7_1_function_access()
> > +    local _func = box.space._func
> > +    local _priv = box.space._priv
> > +    local datetime = os.date("%Y-%m-%d %H:%M:%S")
> > +    local funcs_to_change = {'LUA', 'box.schema.user.info'}
> > +    for _, name in pairs(funcs_to_change) do
> > +        local func = _func.index['name']:get(name)
> > +        if func ~= nil and func.setuid ~= 0 then
> > +            local id = func.id
> > +            log.info('remove old function "'..name..'"')
> > +            _priv:delete({2, 'function', id})
> > +            _func:delete({id})
> > +            log.info('create function "'..name..'" with unset setuid')
> > +            local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime},
> > +                                          {'=', 19, datetime}})
> > +            _func:replace(new_func)
> > +            log.info('grant execute on function "'..name..'" to public')
> > +            _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X}
> > +        end
> > +    end
> > +end
> > +
> > +local function upgrade_to_2_7_1()
> > +    backport_upgrade_2_7_1_function_access()
> > +end
> > +
> > --------------------------------------------------------------------------------
> > 
> > local handlers = {
> > @@ -985,6 +1014,7 @@ local handlers = {
> >     {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
> >     {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
> >     {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
> > +    {version = mkversion(2, 7, 1), func = upgrade_to_2_7_1, auto = true},
> > }
> > 
> > -- Schema version of the snapshot.
> > diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
> > index 0876e77a6..ed7accea3 100644
> > --- a/test/box-py/bootstrap.result
> > +++ b/test/box-py/bootstrap.result
> > @@ -4,7 +4,7 @@ box.internal.bootstrap()
> > box.space._schema:select{}
> > ---
> > - - ['max_id', 511]
> > -  - ['version', 2, 3, 1]
> > +  - ['version', 2, 7, 1]
> > ...
> > box.space._cluster:select{}
> > ---
> > @@ -167,7 +167,7 @@ box.space._user:select{}
> > ...
> > for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end
> > ---
> > -- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none',
> > +- - [1, 1, 'box.schema.user.info', 0, 'LUA', '', 'function', [], 'any', 'none', 'none',
> >     false, false, true, ['LUA'], {}, '', '', '']
> > ...
> > box.space._priv:select{}
> > diff --git a/test/box/access.result b/test/box/access.result
> > index 20b1b8b35..27e636122 100644
> > --- a/test/box/access.result
> > +++ b/test/box/access.result
> > @@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
> > sp:drop()
> > ---
> > ...
> > +--
> > +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
> > +-- excess rights.
> > +--
> > +_ = box.schema.func.call("LUA", "return 1")
> > +---
> > +...
> > +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> > +---
> > +...
> > +_ = box.schema.func.call("box.schema.user.info", 0)
> > +---
> > +...
> > +_ = box.schema.func.call("box.schema.user.info", 1)
> > +---
> > +...
> > +session.su('guest')
> > +---
> > +...
> > +_ = box.schema.func.call("LUA", "return 1")
> > +---
> > +...
> > +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> > +---
> > +- error: Read access to space '_space' is denied for user 'guest'
> > +...
> > +_ = box.schema.func.call("box.schema.user.info", 0)
> > +---
> > +...
> > +_ = box.schema.func.call("box.schema.user.info", 1)
> > +---
> > +- error: User '1' is not found
> > +...
> > +session.su('admin')
> > +---
> > +...
> > diff --git a/test/box/access.test.lua b/test/box/access.test.lua
> > index 3e083a383..a62f87ad8 100644
> > --- a/test/box/access.test.lua
> > +++ b/test/box/access.test.lua
> > @@ -824,3 +824,18 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
> > box.schema.user.revoke('guest', 'read,write,execute', 'universe')
> > box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
> > sp:drop()
> > +
> > +--
> > +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
> > +-- excess rights.
> > +--
> > +_ = box.schema.func.call("LUA", "return 1")
> > +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> > +_ = box.schema.func.call("box.schema.user.info", 0)
> > +_ = box.schema.func.call("box.schema.user.info", 1)
> > +session.su('guest')
> > +_ = box.schema.func.call("LUA", "return 1")
> > +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> > +_ = box.schema.func.call("box.schema.user.info", 0)
> > +_ = box.schema.func.call("box.schema.user.info", 1)
> > +session.su('admin')
> > -- 
> > 2.25.1
> > 
> 


From 21efff563826e47ea049aff2c7215b5bac5883e6 Mon Sep 17 00:00:00 2001
Message-Id: <21efff563826e47ea049aff2c7215b5bac5883e6.1608658852.git.imeevma@gmail.com>
From: Mergen Imeev <imeevma@gmail.com>
Date: Mon, 2 Nov 2020 13:21:58 +0300
Subject: [PATCH v1 1/1] box: remove unnecessary rights from peristent
 functions

After this patch, the persistent functions "box.schema.user.info" and
"LUA" will have the same rights as the user who executed them.

The problem was that setuid was unnecessarily set. Because of this,
these functions had the same rights as the user who created them.
However, they must have the same rights as the user who used them.

Fixes tarantool/security#1
---
https://github.com/tarantool/security/issues/1
https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access

@ChangeLog
- The functions "LUA" and "box.schema.user.info" now have the
  same rights as the user who executes them. (gh-security-1).


 src/box/bootstrap.snap       | Bin 5976 -> 5991 bytes
 src/box/lua/upgrade.lua      |  30 +++++++++++++++++++++++++++++
 test/box-py/bootstrap.result |   4 ++--
 test/box/access.result       |  36 +++++++++++++++++++++++++++++++++++
 test/box/access.test.lua     |  15 +++++++++++++++
 5 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index 8bd4f7ce24216a8bcced6aa97c20c83eb7a02c77..c4a70297aad138d426a24ee4447af485e3597536 100644

diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index a86a0d410..6fba260bd 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -971,6 +971,35 @@ local function upgrade_to_2_3_1()
     create_session_settings_space()
 end
 
+--------------------------------------------------------------------------------
+-- Tarantool 2.7.1
+--------------------------------------------------------------------------------
+local function function_access()
+    local _func = box.space._func
+    local _priv = box.space._priv
+    local datetime = os.date("%Y-%m-%d %H:%M:%S")
+    local funcs_to_change = {'LUA', 'box.schema.user.info'}
+    for _, name in pairs(funcs_to_change) do
+        local func = _func.index['name']:get(name)
+        if func ~= nil and func.setuid ~= 0 then
+            local id = func.id
+            log.info('remove old function "'..name..'"')
+            _priv:delete({2, 'function', id})
+            _func:delete({id})
+            log.info('create function "'..name..'" with unset setuid')
+            local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime},
+                                          {'=', 19, datetime}})
+            _func:replace(new_func)
+            log.info('grant execute on function "'..name..'" to public')
+            _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X}
+        end
+    end
+end
+
+local function upgrade_to_2_7_1()
+    function_access()
+end
+
 --------------------------------------------------------------------------------
 
 local handlers = {
@@ -985,6 +1014,7 @@ local handlers = {
     {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
     {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
     {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
+    {version = mkversion(2, 7, 1), func = upgrade_to_2_7_1, auto = true},
 }
 
 -- Schema version of the snapshot.
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 0876e77a6..ed7accea3 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -4,7 +4,7 @@ box.internal.bootstrap()
 box.space._schema:select{}
 ---
 - - ['max_id', 511]
-  - ['version', 2, 3, 1]
+  - ['version', 2, 7, 1]
 ...
 box.space._cluster:select{}
 ---
@@ -167,7 +167,7 @@ box.space._user:select{}
 ...
 for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end
 ---
-- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none',
+- - [1, 1, 'box.schema.user.info', 0, 'LUA', '', 'function', [], 'any', 'none', 'none',
     false, false, true, ['LUA'], {}, '', '', '']
 ...
 box.space._priv:select{}
diff --git a/test/box/access.result b/test/box/access.result
index 20b1b8b35..27e636122 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
 ---
 ...
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+...
+session.su('guest')
+---
+...
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+---
+- error: Read access to space '_space' is denied for user 'guest'
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+- error: User '1' is not found
+...
+session.su('admin')
+---
+...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 3e083a383..a62f87ad8 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -824,3 +824,18 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
+
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('guest')
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('admin')
-- 
2.25.1

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
  2020-12-22  7:56 imeevma
@ 2020-12-22  8:13 ` Sergey Ostanevich
  0 siblings, 0 replies; 15+ messages in thread
From: Sergey Ostanevich @ 2020-12-22  8:13 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Hi!

Thanks for the patch, LGTM.

Sergos

> On 22 Dec 2020, at 10:56, Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org> wrote:
> 
> After this patch, the persistent functions "box.schema.user.info" and
> "LUA" will have the same rights as the user who executed them.
> 
> The problem was that setuid was unnecessarily set. Because of this,
> these functions had the same rights as the user who created them.
> However, they must have the same rights as the user who used them.
> 
> Part of tarantool/security#1
> ---
> https://github.com/tarantool/security/issues/1
> https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access-2_5
> 
> src/box/bootstrap.snap       | Bin 5976 -> 5975 bytes
> src/box/lua/upgrade.lua      |  30 +++++++++++++++++++++++++++++
> test/box-py/bootstrap.result |   4 ++--
> test/box/access.result       |  36 +++++++++++++++++++++++++++++++++++
> test/box/access.test.lua     |  15 +++++++++++++++
> 5 files changed, 83 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
> index 8bd4f7ce24216a8bcced6aa97c20c83eb7a02c77..49203ad568ee549ce63ea059775cb74369f407f6 100644
> 
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index add791cd7..e5ddd513f 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -971,6 +971,35 @@ local function upgrade_to_2_3_1()
>     create_session_settings_space()
> end
> 
> +--------------------------------------------------------------------------------
> +-- Tarantool 2.5.3
> +--------------------------------------------------------------------------------
> +local function backport_upgrade_2_7_1_function_access()
> +    local _func = box.space._func
> +    local _priv = box.space._priv
> +    local datetime = os.date("%Y-%m-%d %H:%M:%S")
> +    local funcs_to_change = {'LUA', 'box.schema.user.info'}
> +    for _, name in pairs(funcs_to_change) do
> +        local func = _func.index['name']:get(name)
> +        if func ~= nil and func.setuid ~= 0 then
> +            local id = func.id
> +            log.info('remove old function "'..name..'"')
> +            _priv:delete({2, 'function', id})
> +            _func:delete({id})
> +            log.info('create function "'..name..'" with unset setuid')
> +            local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime},
> +                                          {'=', 19, datetime}})
> +            _func:replace(new_func)
> +            log.info('grant execute on function "'..name..'" to public')
> +            _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X}
> +        end
> +    end
> +end
> +
> +local function upgrade_to_2_5_3()
> +    backport_upgrade_2_7_1_function_access()
> +end
> +
> --------------------------------------------------------------------------------
> 
> local function get_version()
> @@ -1007,6 +1036,7 @@ local function upgrade(options)
>         {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
>         {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
>         {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
> +        {version = mkversion(2, 5, 3), func = upgrade_to_2_5_3, auto = true},
>     }
> 
>     for _, handler in ipairs(handlers) do
> diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
> index 0876e77a6..9ebccc56d 100644
> --- a/test/box-py/bootstrap.result
> +++ b/test/box-py/bootstrap.result
> @@ -4,7 +4,7 @@ box.internal.bootstrap()
> box.space._schema:select{}
> ---
> - - ['max_id', 511]
> -  - ['version', 2, 3, 1]
> +  - ['version', 2, 5, 3]
> ...
> box.space._cluster:select{}
> ---
> @@ -167,7 +167,7 @@ box.space._user:select{}
> ...
> for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end
> ---
> -- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none',
> +- - [1, 1, 'box.schema.user.info', 0, 'LUA', '', 'function', [], 'any', 'none', 'none',
>     false, false, true, ['LUA'], {}, '', '', '']
> ...
> box.space._priv:select{}
> diff --git a/test/box/access.result b/test/box/access.result
> index 20b1b8b35..27e636122 100644
> --- a/test/box/access.result
> +++ b/test/box/access.result
> @@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
> sp:drop()
> ---
> ...
> +--
> +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
> +-- excess rights.
> +--
> +_ = box.schema.func.call("LUA", "return 1")
> +---
> +...
> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> +---
> +...
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +---
> +...
> +_ = box.schema.func.call("box.schema.user.info", 1)
> +---
> +...
> +session.su('guest')
> +---
> +...
> +_ = box.schema.func.call("LUA", "return 1")
> +---
> +...
> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> +---
> +- error: Read access to space '_space' is denied for user 'guest'
> +...
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +---
> +...
> +_ = box.schema.func.call("box.schema.user.info", 1)
> +---
> +- error: User '1' is not found
> +...
> +session.su('admin')
> +---
> +...
> diff --git a/test/box/access.test.lua b/test/box/access.test.lua
> index 3e083a383..a62f87ad8 100644
> --- a/test/box/access.test.lua
> +++ b/test/box/access.test.lua
> @@ -824,3 +824,18 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
> box.schema.user.revoke('guest', 'read,write,execute', 'universe')
> box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
> sp:drop()
> +
> +--
> +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
> +-- excess rights.
> +--
> +_ = box.schema.func.call("LUA", "return 1")
> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +_ = box.schema.func.call("box.schema.user.info", 1)
> +session.su('guest')
> +_ = box.schema.func.call("LUA", "return 1")
> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +_ = box.schema.func.call("box.schema.user.info", 1)
> +session.su('admin')
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
@ 2020-12-22  7:56 imeevma
  2020-12-22  8:13 ` Sergey Ostanevich
  0 siblings, 1 reply; 15+ messages in thread
From: imeevma @ 2020-12-22  7:56 UTC (permalink / raw)
  To: s.ostanevich; +Cc: tarantool-patches

After this patch, the persistent functions "box.schema.user.info" and
"LUA" will have the same rights as the user who executed them.

The problem was that setuid was unnecessarily set. Because of this,
these functions had the same rights as the user who created them.
However, they must have the same rights as the user who used them.

Part of tarantool/security#1
---
https://github.com/tarantool/security/issues/1
https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access-2_5

 src/box/bootstrap.snap       | Bin 5976 -> 5975 bytes
 src/box/lua/upgrade.lua      |  30 +++++++++++++++++++++++++++++
 test/box-py/bootstrap.result |   4 ++--
 test/box/access.result       |  36 +++++++++++++++++++++++++++++++++++
 test/box/access.test.lua     |  15 +++++++++++++++
 5 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index 8bd4f7ce24216a8bcced6aa97c20c83eb7a02c77..49203ad568ee549ce63ea059775cb74369f407f6 100644

diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index add791cd7..e5ddd513f 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -971,6 +971,35 @@ local function upgrade_to_2_3_1()
     create_session_settings_space()
 end
 
+--------------------------------------------------------------------------------
+-- Tarantool 2.5.3
+--------------------------------------------------------------------------------
+local function backport_upgrade_2_7_1_function_access()
+    local _func = box.space._func
+    local _priv = box.space._priv
+    local datetime = os.date("%Y-%m-%d %H:%M:%S")
+    local funcs_to_change = {'LUA', 'box.schema.user.info'}
+    for _, name in pairs(funcs_to_change) do
+        local func = _func.index['name']:get(name)
+        if func ~= nil and func.setuid ~= 0 then
+            local id = func.id
+            log.info('remove old function "'..name..'"')
+            _priv:delete({2, 'function', id})
+            _func:delete({id})
+            log.info('create function "'..name..'" with unset setuid')
+            local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime},
+                                          {'=', 19, datetime}})
+            _func:replace(new_func)
+            log.info('grant execute on function "'..name..'" to public')
+            _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X}
+        end
+    end
+end
+
+local function upgrade_to_2_5_3()
+    backport_upgrade_2_7_1_function_access()
+end
+
 --------------------------------------------------------------------------------
 
 local function get_version()
@@ -1007,6 +1036,7 @@ local function upgrade(options)
         {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
         {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
         {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
+        {version = mkversion(2, 5, 3), func = upgrade_to_2_5_3, auto = true},
     }
 
     for _, handler in ipairs(handlers) do
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 0876e77a6..9ebccc56d 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -4,7 +4,7 @@ box.internal.bootstrap()
 box.space._schema:select{}
 ---
 - - ['max_id', 511]
-  - ['version', 2, 3, 1]
+  - ['version', 2, 5, 3]
 ...
 box.space._cluster:select{}
 ---
@@ -167,7 +167,7 @@ box.space._user:select{}
 ...
 for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end
 ---
-- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none',
+- - [1, 1, 'box.schema.user.info', 0, 'LUA', '', 'function', [], 'any', 'none', 'none',
     false, false, true, ['LUA'], {}, '', '', '']
 ...
 box.space._priv:select{}
diff --git a/test/box/access.result b/test/box/access.result
index 20b1b8b35..27e636122 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
 ---
 ...
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+...
+session.su('guest')
+---
+...
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+---
+- error: Read access to space '_space' is denied for user 'guest'
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+- error: User '1' is not found
+...
+session.su('admin')
+---
+...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 3e083a383..a62f87ad8 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -824,3 +824,18 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
+
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('guest')
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('admin')
-- 
2.25.1

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
  2020-12-21 10:51 imeevma
@ 2020-12-21 19:23 ` Sergey Ostanevich
  2020-12-22 17:44   ` Mergen Imeev
  2020-12-23 12:58 ` Kirill Yukhin
  1 sibling, 1 reply; 15+ messages in thread
From: Sergey Ostanevich @ 2020-12-21 19:23 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Hi!

Thanks for the patch!

I wonder why we should have this strange naming for the function of
backport 2_7_1 from 2_7_1 itself. Perhaps just the ‘function_access’
is enough?

Otherwise LGTM.

Sergos


> On 21 Dec 2020, at 13:51, Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org> wrote:
> 
> After this patch, the persistent functions "box.schema.user.info" and
> "LUA" will have the same rights as the user who executed them.
> 
> The problem was that setuid was unnecessarily set. Because of this,
> these functions had the same rights as the user who created them.
> However, they must have the same rights as the user who used them.
> 
> Fixes tarantool/security#1
> ---
> https://github.com/tarantool/security/issues/1
> https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access
> 
> @ChangeLog
> - The functions "LUA" and "box.schema.user.info" now have the
>   same rights as the user who executes them. (gh-security-1).
> 
> 
> src/box/bootstrap.snap       | Bin 5976 -> 5991 bytes
> src/box/lua/upgrade.lua      |  30 +++++++++++++++++++++++++++++
> test/box-py/bootstrap.result |   4 ++--
> test/box/access.result       |  36 +++++++++++++++++++++++++++++++++++
> test/box/access.test.lua     |  15 +++++++++++++++
> 5 files changed, 83 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
> index 8bd4f7ce24216a8bcced6aa97c20c83eb7a02c77..9e7a4e3291accfc387b315f85be9b01f443048fd 100644
> 
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index a86a0d410..09cd015f0 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -971,6 +971,35 @@ local function upgrade_to_2_3_1()
>     create_session_settings_space()
> end
> 
> +--------------------------------------------------------------------------------
> +-- Tarantool 2.7.1
> +--------------------------------------------------------------------------------
> +local function backport_upgrade_2_7_1_function_access()
> +    local _func = box.space._func
> +    local _priv = box.space._priv
> +    local datetime = os.date("%Y-%m-%d %H:%M:%S")
> +    local funcs_to_change = {'LUA', 'box.schema.user.info'}
> +    for _, name in pairs(funcs_to_change) do
> +        local func = _func.index['name']:get(name)
> +        if func ~= nil and func.setuid ~= 0 then
> +            local id = func.id
> +            log.info('remove old function "'..name..'"')
> +            _priv:delete({2, 'function', id})
> +            _func:delete({id})
> +            log.info('create function "'..name..'" with unset setuid')
> +            local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime},
> +                                          {'=', 19, datetime}})
> +            _func:replace(new_func)
> +            log.info('grant execute on function "'..name..'" to public')
> +            _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X}
> +        end
> +    end
> +end
> +
> +local function upgrade_to_2_7_1()
> +    backport_upgrade_2_7_1_function_access()
> +end
> +
> --------------------------------------------------------------------------------
> 
> local handlers = {
> @@ -985,6 +1014,7 @@ local handlers = {
>     {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
>     {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
>     {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
> +    {version = mkversion(2, 7, 1), func = upgrade_to_2_7_1, auto = true},
> }
> 
> -- Schema version of the snapshot.
> diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
> index 0876e77a6..ed7accea3 100644
> --- a/test/box-py/bootstrap.result
> +++ b/test/box-py/bootstrap.result
> @@ -4,7 +4,7 @@ box.internal.bootstrap()
> box.space._schema:select{}
> ---
> - - ['max_id', 511]
> -  - ['version', 2, 3, 1]
> +  - ['version', 2, 7, 1]
> ...
> box.space._cluster:select{}
> ---
> @@ -167,7 +167,7 @@ box.space._user:select{}
> ...
> for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end
> ---
> -- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none',
> +- - [1, 1, 'box.schema.user.info', 0, 'LUA', '', 'function', [], 'any', 'none', 'none',
>     false, false, true, ['LUA'], {}, '', '', '']
> ...
> box.space._priv:select{}
> diff --git a/test/box/access.result b/test/box/access.result
> index 20b1b8b35..27e636122 100644
> --- a/test/box/access.result
> +++ b/test/box/access.result
> @@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
> sp:drop()
> ---
> ...
> +--
> +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
> +-- excess rights.
> +--
> +_ = box.schema.func.call("LUA", "return 1")
> +---
> +...
> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> +---
> +...
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +---
> +...
> +_ = box.schema.func.call("box.schema.user.info", 1)
> +---
> +...
> +session.su('guest')
> +---
> +...
> +_ = box.schema.func.call("LUA", "return 1")
> +---
> +...
> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> +---
> +- error: Read access to space '_space' is denied for user 'guest'
> +...
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +---
> +...
> +_ = box.schema.func.call("box.schema.user.info", 1)
> +---
> +- error: User '1' is not found
> +...
> +session.su('admin')
> +---
> +...
> diff --git a/test/box/access.test.lua b/test/box/access.test.lua
> index 3e083a383..a62f87ad8 100644
> --- a/test/box/access.test.lua
> +++ b/test/box/access.test.lua
> @@ -824,3 +824,18 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
> box.schema.user.revoke('guest', 'read,write,execute', 'universe')
> box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
> sp:drop()
> +
> +--
> +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
> +-- excess rights.
> +--
> +_ = box.schema.func.call("LUA", "return 1")
> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +_ = box.schema.func.call("box.schema.user.info", 1)
> +session.su('guest')
> +_ = box.schema.func.call("LUA", "return 1")
> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +_ = box.schema.func.call("box.schema.user.info", 1)
> +session.su('admin')
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
  2020-12-21 11:38 imeevma
@ 2020-12-21 19:14 ` Sergey Ostanevich
  0 siblings, 0 replies; 15+ messages in thread
From: Sergey Ostanevich @ 2020-12-21 19:14 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Hi! 

Thanks for the patch!
aaa
LGTM.

Sergos


> On 21 Dec 2020, at 14:38, imeevma@tarantool.org wrote:
> 
> After this patch, the persistent functions "box.schema.user.info" and
> "LUA" will have the same rights as the user who executed them.
> 
> The problem was that setuid was unnecessarily set. Because of this,
> these functions had the same rights as the user who created them.
> However, they must have the same rights as the user who used them.
> 
> Part of tarantool/security#1
> ---
> https://github.com/tarantool/security/issues/1
> https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access-2_6
> 
> 
> src/box/bootstrap.snap       | Bin 5976 -> 5997 bytes
> src/box/lua/upgrade.lua      |  30 +++++++++++++++++++++++++++++
> test/box-py/bootstrap.result |   4 ++--
> test/box/access.result       |  36 +++++++++++++++++++++++++++++++++++
> test/box/access.test.lua     |  15 +++++++++++++++
> 5 files changed, 83 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
> index 8bd4f7ce24216a8bcced6aa97c20c83eb7a02c77..b9d84d7abd9dba0dd5ca72438c69cc6a5c5f0470 100644
> 
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index add791cd7..be1a0d47d 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -971,6 +971,35 @@ local function upgrade_to_2_3_1()
>     create_session_settings_space()
> end
> 
> +--------------------------------------------------------------------------------
> +-- Tarantool 2.6.2
> +--------------------------------------------------------------------------------
> +local function backport_upgrade_2_7_1_function_access()
> +    local _func = box.space._func
> +    local _priv = box.space._priv
> +    local datetime = os.date("%Y-%m-%d %H:%M:%S")
> +    local funcs_to_change = {'LUA', 'box.schema.user.info'}
> +    for _, name in pairs(funcs_to_change) do
> +        local func = _func.index['name']:get(name)
> +        if func ~= nil and func.setuid ~= 0 then
> +            local id = func.id
> +            log.info('remove old function "'..name..'"')
> +            _priv:delete({2, 'function', id})
> +            _func:delete({id})
> +            log.info('create function "'..name..'" with unset setuid')
> +            local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime},
> +                                          {'=', 19, datetime}})
> +            _func:replace(new_func)
> +            log.info('grant execute on function "'..name..'" to public')
> +            _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X}
> +        end
> +    end
> +end
> +
> +local function upgrade_to_2_6_2()
> +    backport_upgrade_2_7_1_function_access()
> +end
> +
> --------------------------------------------------------------------------------
> 
> local function get_version()
> @@ -1007,6 +1036,7 @@ local function upgrade(options)
>         {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
>         {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
>         {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
> +        {version = mkversion(2, 6, 2), func = upgrade_to_2_6_2, auto = true},
>     }
> 
>     for _, handler in ipairs(handlers) do
> diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
> index 0876e77a6..a371bb369 100644
> --- a/test/box-py/bootstrap.result
> +++ b/test/box-py/bootstrap.result
> @@ -4,7 +4,7 @@ box.internal.bootstrap()
> box.space._schema:select{}
> ---
> - - ['max_id', 511]
> -  - ['version', 2, 3, 1]
> +  - ['version', 2, 6, 2]
> ...
> box.space._cluster:select{}
> ---
> @@ -167,7 +167,7 @@ box.space._user:select{}
> ...
> for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end
> ---
> -- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none',
> +- - [1, 1, 'box.schema.user.info', 0, 'LUA', '', 'function', [], 'any', 'none', 'none',
>     false, false, true, ['LUA'], {}, '', '', '']
> ...
> box.space._priv:select{}
> diff --git a/test/box/access.result b/test/box/access.result
> index 20b1b8b35..27e636122 100644
> --- a/test/box/access.result
> +++ b/test/box/access.result
> @@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
> sp:drop()
> ---
> ...
> +--
> +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
> +-- excess rights.
> +--
> +_ = box.schema.func.call("LUA", "return 1")
> +---
> +...
> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> +---
> +...
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +---
> +...
> +_ = box.schema.func.call("box.schema.user.info", 1)
> +---
> +...
> +session.su('guest')
> +---
> +...
> +_ = box.schema.func.call("LUA", "return 1")
> +---
> +...
> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> +---
> +- error: Read access to space '_space' is denied for user 'guest'
> +...
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +---
> +...
> +_ = box.schema.func.call("box.schema.user.info", 1)
> +---
> +- error: User '1' is not found
> +...
> +session.su('admin')
> +---
> +...
> diff --git a/test/box/access.test.lua b/test/box/access.test.lua
> index 3e083a383..a62f87ad8 100644
> --- a/test/box/access.test.lua
> +++ b/test/box/access.test.lua
> @@ -824,3 +824,18 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
> box.schema.user.revoke('guest', 'read,write,execute', 'universe')
> box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
> sp:drop()
> +
> +--
> +-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
> +-- excess rights.
> +--
> +_ = box.schema.func.call("LUA", "return 1")
> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +_ = box.schema.func.call("box.schema.user.info", 1)
> +session.su('guest')
> +_ = box.schema.func.call("LUA", "return 1")
> +_ = box.schema.func.call("LUA", "return box.space._space:count()")
> +_ = box.schema.func.call("box.schema.user.info", 0)
> +_ = box.schema.func.call("box.schema.user.info", 1)
> +session.su('admin')
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
@ 2020-12-21 11:38 imeevma
  2020-12-21 19:14 ` Sergey Ostanevich
  0 siblings, 1 reply; 15+ messages in thread
From: imeevma @ 2020-12-21 11:38 UTC (permalink / raw)
  To: s.ostanevich; +Cc: tarantool-patches

After this patch, the persistent functions "box.schema.user.info" and
"LUA" will have the same rights as the user who executed them.

The problem was that setuid was unnecessarily set. Because of this,
these functions had the same rights as the user who created them.
However, they must have the same rights as the user who used them.

Part of tarantool/security#1
---
https://github.com/tarantool/security/issues/1
https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access-2_6


 src/box/bootstrap.snap       | Bin 5976 -> 5997 bytes
 src/box/lua/upgrade.lua      |  30 +++++++++++++++++++++++++++++
 test/box-py/bootstrap.result |   4 ++--
 test/box/access.result       |  36 +++++++++++++++++++++++++++++++++++
 test/box/access.test.lua     |  15 +++++++++++++++
 5 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index 8bd4f7ce24216a8bcced6aa97c20c83eb7a02c77..b9d84d7abd9dba0dd5ca72438c69cc6a5c5f0470 100644

diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index add791cd7..be1a0d47d 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -971,6 +971,35 @@ local function upgrade_to_2_3_1()
     create_session_settings_space()
 end
 
+--------------------------------------------------------------------------------
+-- Tarantool 2.6.2
+--------------------------------------------------------------------------------
+local function backport_upgrade_2_7_1_function_access()
+    local _func = box.space._func
+    local _priv = box.space._priv
+    local datetime = os.date("%Y-%m-%d %H:%M:%S")
+    local funcs_to_change = {'LUA', 'box.schema.user.info'}
+    for _, name in pairs(funcs_to_change) do
+        local func = _func.index['name']:get(name)
+        if func ~= nil and func.setuid ~= 0 then
+            local id = func.id
+            log.info('remove old function "'..name..'"')
+            _priv:delete({2, 'function', id})
+            _func:delete({id})
+            log.info('create function "'..name..'" with unset setuid')
+            local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime},
+                                          {'=', 19, datetime}})
+            _func:replace(new_func)
+            log.info('grant execute on function "'..name..'" to public')
+            _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X}
+        end
+    end
+end
+
+local function upgrade_to_2_6_2()
+    backport_upgrade_2_7_1_function_access()
+end
+
 --------------------------------------------------------------------------------
 
 local function get_version()
@@ -1007,6 +1036,7 @@ local function upgrade(options)
         {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
         {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
         {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
+        {version = mkversion(2, 6, 2), func = upgrade_to_2_6_2, auto = true},
     }
 
     for _, handler in ipairs(handlers) do
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 0876e77a6..a371bb369 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -4,7 +4,7 @@ box.internal.bootstrap()
 box.space._schema:select{}
 ---
 - - ['max_id', 511]
-  - ['version', 2, 3, 1]
+  - ['version', 2, 6, 2]
 ...
 box.space._cluster:select{}
 ---
@@ -167,7 +167,7 @@ box.space._user:select{}
 ...
 for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end
 ---
-- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none',
+- - [1, 1, 'box.schema.user.info', 0, 'LUA', '', 'function', [], 'any', 'none', 'none',
     false, false, true, ['LUA'], {}, '', '', '']
 ...
 box.space._priv:select{}
diff --git a/test/box/access.result b/test/box/access.result
index 20b1b8b35..27e636122 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
 ---
 ...
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+...
+session.su('guest')
+---
+...
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+---
+- error: Read access to space '_space' is denied for user 'guest'
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+- error: User '1' is not found
+...
+session.su('admin')
+---
+...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 3e083a383..a62f87ad8 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -824,3 +824,18 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
+
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('guest')
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('admin')
-- 
2.25.1

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions
@ 2020-12-21 10:51 imeevma
  2020-12-21 19:23 ` Sergey Ostanevich
  2020-12-23 12:58 ` Kirill Yukhin
  0 siblings, 2 replies; 15+ messages in thread
From: imeevma @ 2020-12-21 10:51 UTC (permalink / raw)
  To: s.ostanevich; +Cc: tarantool-patches

After this patch, the persistent functions "box.schema.user.info" and
"LUA" will have the same rights as the user who executed them.

The problem was that setuid was unnecessarily set. Because of this,
these functions had the same rights as the user who created them.
However, they must have the same rights as the user who used them.

Fixes tarantool/security#1
---
https://github.com/tarantool/security/issues/1
https://github.com/tarantool/tarantool/tree/imeevma/gh-security-1-lua-function-access

@ChangeLog
 - The functions "LUA" and "box.schema.user.info" now have the
   same rights as the user who executes them. (gh-security-1).


 src/box/bootstrap.snap       | Bin 5976 -> 5991 bytes
 src/box/lua/upgrade.lua      |  30 +++++++++++++++++++++++++++++
 test/box-py/bootstrap.result |   4 ++--
 test/box/access.result       |  36 +++++++++++++++++++++++++++++++++++
 test/box/access.test.lua     |  15 +++++++++++++++
 5 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index 8bd4f7ce24216a8bcced6aa97c20c83eb7a02c77..9e7a4e3291accfc387b315f85be9b01f443048fd 100644

diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index a86a0d410..09cd015f0 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -971,6 +971,35 @@ local function upgrade_to_2_3_1()
     create_session_settings_space()
 end
 
+--------------------------------------------------------------------------------
+-- Tarantool 2.7.1
+--------------------------------------------------------------------------------
+local function backport_upgrade_2_7_1_function_access()
+    local _func = box.space._func
+    local _priv = box.space._priv
+    local datetime = os.date("%Y-%m-%d %H:%M:%S")
+    local funcs_to_change = {'LUA', 'box.schema.user.info'}
+    for _, name in pairs(funcs_to_change) do
+        local func = _func.index['name']:get(name)
+        if func ~= nil and func.setuid ~= 0 then
+            local id = func.id
+            log.info('remove old function "'..name..'"')
+            _priv:delete({2, 'function', id})
+            _func:delete({id})
+            log.info('create function "'..name..'" with unset setuid')
+            local new_func = func:update({{'=', 4, 0}, {'=', 18, datetime},
+                                          {'=', 19, datetime}})
+            _func:replace(new_func)
+            log.info('grant execute on function "'..name..'" to public')
+            _priv:replace{ADMIN, PUBLIC, 'function', id, box.priv.X}
+        end
+    end
+end
+
+local function upgrade_to_2_7_1()
+    backport_upgrade_2_7_1_function_access()
+end
+
 --------------------------------------------------------------------------------
 
 local handlers = {
@@ -985,6 +1014,7 @@ local handlers = {
     {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
     {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
     {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
+    {version = mkversion(2, 7, 1), func = upgrade_to_2_7_1, auto = true},
 }
 
 -- Schema version of the snapshot.
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 0876e77a6..ed7accea3 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -4,7 +4,7 @@ box.internal.bootstrap()
 box.space._schema:select{}
 ---
 - - ['max_id', 511]
-  - ['version', 2, 3, 1]
+  - ['version', 2, 7, 1]
 ...
 box.space._cluster:select{}
 ---
@@ -167,7 +167,7 @@ box.space._user:select{}
 ...
 for _, v in box.space._func:pairs{} do r = {} table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end
 ---
-- - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], 'any', 'none', 'none',
+- - [1, 1, 'box.schema.user.info', 0, 'LUA', '', 'function', [], 'any', 'none', 'none',
     false, false, true, ['LUA'], {}, '', '', '']
 ...
 box.space._priv:select{}
diff --git a/test/box/access.result b/test/box/access.result
index 20b1b8b35..27e636122 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -2141,3 +2141,39 @@ box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
 ---
 ...
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+...
+session.su('guest')
+---
+...
+_ = box.schema.func.call("LUA", "return 1")
+---
+...
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+---
+- error: Read access to space '_space' is denied for user 'guest'
+...
+_ = box.schema.func.call("box.schema.user.info", 0)
+---
+...
+_ = box.schema.func.call("box.schema.user.info", 1)
+---
+- error: User '1' is not found
+...
+session.su('admin')
+---
+...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 3e083a383..a62f87ad8 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -824,3 +824,18 @@ box.schema.user.grant('guest', 'read,write,execute', 'space', 'not_universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 box.schema.user.revoke('guest', 'read,write,execute', 'space', 'not_universe')
 sp:drop()
+
+--
+-- Make sure that the functions "LUA" and "box.schema.user.info" do not have
+-- excess rights.
+--
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('guest')
+_ = box.schema.func.call("LUA", "return 1")
+_ = box.schema.func.call("LUA", "return box.space._space:count()")
+_ = box.schema.func.call("box.schema.user.info", 0)
+_ = box.schema.func.call("box.schema.user.info", 1)
+session.su('admin')
-- 
2.25.1

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-12-23 12:58 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-03  0:03 [Tarantool-patches] [PATCH v1 1/1] box: remove unnecessary rights from peristent functions imeevma
2020-11-11 21:48 ` Vladislav Shpilevoy
2020-12-03  9:54   ` Mergen Imeev
2020-12-04 23:10     ` Vladislav Shpilevoy
2020-12-16 20:23       ` Mergen Imeev
2020-12-20 14:05         ` Vladislav Shpilevoy
2020-12-21 10:51 imeevma
2020-12-21 19:23 ` Sergey Ostanevich
2020-12-22 17:44   ` Mergen Imeev
2020-12-23  9:44     ` Sergey Ostanevich
2020-12-23 12:58 ` Kirill Yukhin
2020-12-21 11:38 imeevma
2020-12-21 19:14 ` Sergey Ostanevich
2020-12-22  7:56 imeevma
2020-12-22  8:13 ` Sergey Ostanevich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox