13
\$\begingroup\$

For a school project I'm working on a small library to do steganography. I've just finished the first method of steganography and thought I'd get some feedback.

This steganography method works by changing the last bit of each channel of each pixel to a bit of the file. Reading the file from the image does the reverse operation.

The first point I'm unsure about, is my parameter and return types. The user will be able to hide any file in an image, so I'm currently asking for a Bitmap and a String with the file location. Should I ask for two file paths? Or should I ask for a Bitmap and a Path? I'm not sure. And what about the return types? Should I be giving back a byte[], or should I give back a file?

Besides that, general comments are welcome as well. Should anyone need it, the entire project is on GitHub.

public class SteganoBMP
    {
        //Embed a file in an image. Because Bitmap is an abstract representation of an image,
        //it can then be saved in any image format. Though the file will not be retrievable from
        //a lossy format.
        public static Bitmap Embed(Bitmap target, String inputFilePath)
        {
            BitmapData bmpData = PrepareImage(target);

            //Math.Abs because Stride can be negative if the image is saved upside down
            //i.e. The first pixel in memory is the bottom right one.
            int imageSize = Math.Abs(bmpData.Stride) * bmpData.Height;

            //Get all the bytes from the file we want to embed, and save it in a byte array
            byte[] fileBytes = File.ReadAllBytes(inputFilePath);

            //If the file we want to embed is larger than 8 times the size of the image, we can't store it.
            //This is because We need one byte in the image to store each bit of the file
            if (fileBytes.Length * 8 > imageSize)
                throw new FileTooLargeException("The file you are trying to embed needs an image of at least" + fileBytes.Length * 8 +  "bytes large");

            bool fileWritten = false;
            int lastByte = 0;

            unsafe
            {
                byte* ptr = (byte*)bmpData.Scan0;

                for (int i = 0; i < imageSize; i++)
                {
                    //We need to do this step 8 times per iteration because we need to access 8 bytes
                    //in the image for each byte in the file.
                    for (int j = 0; j < 8; j++)
                    {
                        //Go out of the loop once we've written the entire file.
                        if (i >= fileBytes.Length)
                        {
                            fileWritten = true;
                            break;
                        }
                        //AND the current value with ~1 (inverse of 1: 11111110). 
                        //This wil set the last bit to 0.
                        //Then OR with the new bit
                        //Helper.GetBitAsByte extracts a single bit from a byte.
                        //And converts it to a byte, so we can do boolean arithmetic with it.
                        ptr[i * 8 + j] = (byte)(ptr[i * 8 + j] & ~1 | Helper.GetBitAsByte(fileBytes[i], 7 - j));
                    }

                    //We've now written all bytes, remember position so we can write EOF there.
                    if (fileWritten)
                    {
                        lastByte = i;
                        break;
                    }
                }

                //Write an EOF character (0xff) after our data.
                //This is the last byte in our file, so we've used 8 times as many in our image, hence times 8
                for (int i = lastByte * 8; i < lastByte * 8 + 8; i++)
                {
                    ptr[i] = (byte)(ptr[i] | 1);
                }
            }

            target.UnlockBits(bmpData);

            return target;
        }

        //Extract an embedded file from an image.
        //Returns a byte[], so the consumer can do with the data whatever he likes.
        public static byte[] Extract(Bitmap source)
        {
            BitmapData bmpData = PrepareImage(source);

            int imageSize = Math.Abs(bmpData.Stride) * bmpData.Height;

            //We use a List, because we don't know in advance how big the enbedded file is.
            List<byte> fileBytes = new List<byte>();

            bool fileRead = false;

            unsafe
            {
                //Get a pointer to the first pixel;
                byte* ptr = (byte*)bmpData.Scan0;

                for (int i = 0; i < imageSize; i++)
                {
                    fileBytes.Add(0x0);
                    for (int j = 0; j < 8; j++)
                    {
                        //Set the last bit of the current file byte to the last bit of the image byte.
                        //Then shift it one spot to the left.
                        //This allows us to do this:
                        //0000 0001
                        //0000 0011
                        //0000 0110
                        //0000 1101
                        //etc.
                        fileBytes[i] <<= 1;
                        fileBytes[i] = (byte)(fileBytes[i] & ~1 | Helper.GetBitAsByte(ptr[i * 8 + j], 0));

                        if (fileBytes[i] == 0xff)
                        {
                            fileRead = true;
                            break;
                        }
                    }

                    if (fileRead)
                        break;
                }
            }

            //Remove our last byte, this is the EOF byte.
            fileBytes.RemoveAt(fileBytes.Count - 1);

            //Convert it to a byte array
            byte[] byteArray = fileBytes.ToArray<byte>();

            return byteArray;
        }

        //This is some boilerplate code to prepare our image for unmanaged access.
        private static BitmapData PrepareImage(Bitmap image)
        {
            //We don't require a specific area to be locked, but we still need to specify one
            //So we choose to lock the entire image.
            Rectangle lockArea = new Rectangle(0, 0, image.Width, image.Height);

            //We use LockBits instead of GetPixel, because it is much faster.
            //As a consequence we need to work with unmanaged data.
            BitmapData bmpData = image.LockBits(lockArea,
                System.Drawing.Imaging.ImageLockMode.ReadWrite,
                image.PixelFormat);

            return bmpData;
        }
    }

Helper class:

static class Helper
    {
        //Does a bitshift, but wraps the overflow
        public static byte RotateLeft(byte value, int count)
        {
            return (byte)((value << count) | (value >> (8 - count)));
        }

        //Does a bitshift, but wraps the overflow
        public static byte RotateRight(byte value, int count)
        {
            return (byte)((value >> count) | (value << (8 - count)));
        }

        //Gets the bit at a certain position in a byte
        public static bool GetBit(byte b, int bitNumber)
        {
            return (b & (1 << bitNumber)) != 0;
        }

        //Gets the bit at a certain position in a byte,
        //And presents it as a byte: 00000000 or 00000001
        public static byte GetBitAsByte(byte b, int bitNumber)
        {
            //This is the same function as GetBit, but we add a conversion to Byte.
            //This is possible because a boolean also takes a byte of memory.
            return Convert.ToByte((b & (1 << bitNumber)) != 0);
        }
\$\endgroup\$
3
  • \$\begingroup\$ Can you provide the Helper.GetBitAsByte() method also ? \$\endgroup\$
    – Heslacher
    Sep 24, 2014 at 11:01
  • \$\begingroup\$ @Heslacher Done. I didn't include my Helper class because I got those functions from some very popular SO questions on the topic. \$\endgroup\$ Sep 24, 2014 at 11:03
  • 2
    \$\begingroup\$ TIL steganography is not a typo for stenography... \$\endgroup\$
    – ANeves
    Sep 24, 2014 at 11:24

1 Answer 1

9
\$\begingroup\$

Good

  • Most of the comments are telling why something is done.
  • The input and local used parameter are named good
  • The method- and parameternames fit the naming guidlines

Bad

  • Some of the comments are useless, as they tell what is done like e.g
    //Get all the bytes from the file we want to embed, and save it in a byte array byte[] fileBytes = File.ReadAllBytes(inputFilePath); instead of why something is done.
  • ifstatement without brackets {}
  • The Extract() method does not call UnlockBits()

As you are unsure which inputparameters and which retuntypes to use, why don't you add some overloads of your methods?

public static Bitmap Embed(string imageFilePath, string inputFilePath)
{
    return InternalEmbed(new Bitmap(imageFilePath), File.ReadAllBytes(inputFilePath));
}
public static Bitmap Embed(Bitmap target, string inputFilePath)
{
    return InternalEmbed(target, File.ReadAllBytes(inputFilePath));
}
public static Bitmap Embed(Bitmap target, byte[] content)
{
    return InternalEmbed(target, content);
}  

and rename your former public Embed() method to the private InternalEmbed() method

private  static Bitmap InternalEmbed(Bitmap target, byte[] fileBytes)
{
    // your former code without 
    // byte[] fileBytes = File.ReadAllBytes(inputFilePath);
}        

After thinking about the overloads and looking at the signature of the methods I realized that if we return a Bitmap and also take a Bitmap as input parameter one would not assume that the passed Bitmap would be altered. As we have the overloads taking also a string we can't change the return type to void, therefor we need to call the InternalEmbed() method differently.

public static Bitmap Embed(Bitmap target, String inputFilePath)
{
    return InternalEmbed(new Bitmap(target), File.ReadAllBytes(inputFilePath));
}
public static Bitmap Embed(Bitmap target, byte[] content)
{
    return InternalEmbed(new Bitmap(target), content);
}   

now we don't alter the passed Bitmap anymore.

\$\endgroup\$
2
  • \$\begingroup\$ I really like your idea of overloading the method, thanks! \$\endgroup\$ Sep 24, 2014 at 12:03
  • 2
    \$\begingroup\$ I would also add to the overloads. Instead of working with completely loaded in memory byte arrays, maybe the core method that all the overloads use should use streams. \$\endgroup\$ Sep 24, 2014 at 14:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.