[Ffmpeg-devel] [PATCH] OAEP

[Michael Niedermayer]

Hi

On Thu, Mar 15, 2007 at 06:39:26PM +0100, Vladimir Serbinenko wrote:
> Sorry. A fault of attention. I send the new patch
> Ismail D?nmez a ?crit :
> > On Thursday 15 March 2007 19:06:16 Vladimir Serbinenko wrote:
> >   
> >> Hello Guys. I wrote a patch for supporting OAEP and I also corrected
> >> SHA1 which din't work when compiled in avutil because of be2me_*. I
> >> replaced them by AV_(R/W)B* I also added AV_WB64 to intreadwrite.h. RSA

the OEAP stuff should be in a seperate patch not in the same which fixes
SHA1 bugs


[...]

> Index: sha1.h
> ===================================================================
> --- sha1.h	(r??vision 8386)
> +++ sha1.h	(copie de travail)
> @@ -1,10 +1,15 @@
>  #ifndef AV_SHA1_H
>  #define AV_SHA1_H
>  
> -extern const int av_sha1_size;
> +#define AV_SHA1_SIZE 20
>  
> -struct AVSHA1;
> +typedef struct AVSHA1 {
> +    uint64_t count;
> +    uint8_t buffer[64];
> +    uint32_t state[5];
> +} AVSHA1;

this breaks the seperation of implementation and interface
which requires a seperate disscussion if you think this has some advantages


>  
> +
>  void av_sha1_init(struct AVSHA1* context);

cosmetic change


[...]
> Index: intreadwrite.h
> ===================================================================
> --- intreadwrite.h	(r??vision 8386)
> +++ intreadwrite.h	(copie de travail)
> @@ -69,7 +69,17 @@
>                      ((uint8_t*)(p))[2] = (d)>>8; \
>                      ((uint8_t*)(p))[1] = (d)>>16; \
>                      ((uint8_t*)(p))[0] = (d)>>24; }
> +#define AV_WB64(p, d) { \
> +                    ((uint8_t*)(p))[7] = (d); \
> +                    ((uint8_t*)(p))[6] = (d)>>8; \
> +                    ((uint8_t*)(p))[5] = (d)>>16; \
> +                    ((uint8_t*)(p))[4] = (d)>>24; \
> +                    ((uint8_t*)(p))[3] = (d)>>32; \
> +                    ((uint8_t*)(p))[2] = (d)>>40; \
> +                    ((uint8_t*)(p))[1] = (d)>>48; \
> +                    ((uint8_t*)(p))[0] = (d)>>56; }
>  

this should be in a seperate patch and should rather look like
#define AV_WB64(p, d) { \
    AV_WB32(           p   , (d)>>32)  \
    AV_WB32((uint8_t*)(p)+4,  d     ) }



> +
>  #define AV_RL8(x)  AV_RB8(x)

cosmetic change


[...]
> +/*
> +  We have to calculate:
> +  r=o1^LSHA1(o2)
> +  o2^LSHA1(r)
> +*/
> +/* WARNING: after RSA deciphering first byte is not a part of OAEP */
> +int
> +av_parse_oaep(uint8_t *inbuf, uint8_t *outbuf, int inlen)

comment is not doxygen compatible, also documentation for exported functions
should be in the header not source


> +{
> +  uint8_t *o1, *o2;
> +  uint8_t r[AV_SHA1_SIZE*3];
> +  uint8_t cd[AV_SHA1_SIZE];

indention in ffmpeg should be 4 spaces


> +  uint8_t *tbuf;
> +  int i;
> +  int outlen;
> +  AVSHA1 hashctx;
> +
> +  if(inlen<=AV_SHA1_SIZE)
> +    return -1;
> +  /* Split the message into 2 parts*/
> +  o1=inbuf;
> +  o2=inbuf+AV_SHA1_SIZE;
> +
> +  /* Allocate tbuf*/
> +  tbuf=av_malloc(inlen);

id leave the allocation of the temporary buffer to the caller this would
avoid a dependancy on av_malloc*() (less dependancies are good IMHO, makes
using the code outside libavutil easier)

[...]

> +uint8_t *fast_memcpy(uint8_t *a, uint8_t *b, int n)
> +{
> +  return memcpy(a,b,n);
> +}

this doesnt belong in here


[...]

>  /* (R0+R1), R2, R3, R4 are the different operations used in SHA1 */
> -#define blk0(i) (block[i] = be2me_32(((uint32_t*)buffer)[i]))
> +#define blk0(i) (block[i] = AV_RB32(&((uint32_t*)buffer)[i]))
>  #define blk(i) (block[i] = rol(block[i-3]^block[i-8]^block[i-14]^block[i-16],1))
>  
>  #define R0(v,w,x,y,z,i) z+=((w&(x^y))^y)    +blk0(i)+0x5A827999+rol(v,5);w=rol(w,30);
> @@ -37,7 +31,7 @@
>  #ifdef CONFIG_SMALL
>      for(i=0; i<80; i++){
>          int t;
> -        if(i<16) t= be2me_32(((uint32_t*)buffer)[i]);
> +        if(i<16) t= AV_RB32(((uint32_t*)buffer)[i]);
>          else     t= rol(block[i-3]^block[i-8]^block[i-14]^block[i-16],1);
>          block[i]= t;
>          t+= e+rol(a,5);

this change requires benchmarks and object size comparissions against a
variant which never passes user arrays directly in av_sha1_update()


> @@ -114,7 +108,8 @@
>  
>  void av_sha1_final(AVSHA1* ctx, uint8_t digest[20]){
>      int i;
> -    uint64_t finalcount= be2me_64(ctx->count<<3);
> +    uint64_t finalcount;
> +    AV_WB64(&finalcount,(ctx->count<<3));
>  
>      av_sha1_update(ctx, "\200", 1);
>      while ((ctx->count & 63) != 56) {
> @@ -122,7 +117,7 @@
>      }
>      av_sha1_update(ctx, &finalcount, 8);  /* Should cause a transform() */
>      for(i=0; i<5; i++)
> -        ((uint32_t*)digest)[i]= be2me_32(ctx->state[i]);
> +        AV_WB32(&((uint32_t*)digest)[i],(ctx->state[i]));
>  }

these are is unneeded

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070315/4eaabd25/attachment.pgp>